Skip to content

Conversation

@noahfalk
Copy link
Member

Added clarifying comments on some MethodDesc APIs related to TransientIL and diagnostics.

Fyi @rcj1 @davidwrighton @hoyosjs @jkotas
This is a follow up to #120982

Copilot AI review requested due to automatic review settings October 25, 2025 21:51
Copy link
Contributor

Copilot AI left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Pull Request Overview

This PR adds clarifying documentation comments to MethodDesc APIs in the CoreCLR VM to explain their behavior with respect to TransientIL and diagnostics. The changes help distinguish between different categories of runtime-generated methods (DynamicMethodDesc, TransientIL, ILStubs) and clarify when methods should be hidden from diagnostic tools.

Key Changes

  • Added reminder comment in TryGenerateTransientILImplementation to keep diagnostics filtering in sync when adding new transient IL methods
  • Documented that IsDynamicMethod() only returns true for DynamicMethodDesc, not all runtime-generated IL
  • Clarified that IsILStub() only detects a subset of IL stubs (those using DynamicMethodDesc)

Reviewed Changes

Copilot reviewed 2 out of 2 changed files in this pull request and generated 2 comments.

File Description
src/coreclr/vm/prestub.cpp Adds comment reminding developers to update IsDiagnosticsHidden() when adding new transient IL methods
src/coreclr/vm/method.inl Documents the scope and limitations of IsDynamicMethod(), IsILStub(), and IsDiagnosticsHidden() methods

noahfalk and others added 4 commits October 30, 2025 15:30
Added clarifying comments on some MethodDesc APIs related to TransientIL and diagnostics.
Co-authored-by: Copilot <175728472+Copilot@users.noreply.github.com>
Co-authored-by: Jan Kotas <jkotas@microsoft.com>
@noahfalk noahfalk enabled auto-merge (squash) October 30, 2025 22:33
@noahfalk
Copy link
Member Author

/ba-g this was already good prior to rebasing and its not worthwhile to re-run all the CI again.

@noahfalk noahfalk merged commit c01656e into dotnet:main Oct 30, 2025
16 of 78 checks passed
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Projects

None yet

Development

Successfully merging this pull request may close these issues.

2 participants