Skip to content

Conversation

MichalStrehovsky
Copy link
Member

I don't think anything calls these and because these are virtual, they cannot be trimmed.

Cc @dotnet/ilc-contrib

I don't think anything calls these and because these are virtual, they cannot be trimmed.
@Copilot Copilot AI review requested due to automatic review settings August 14, 2025 10:31
Copy link
Contributor

@Copilot 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 optimizes the generated native format metadata reader by conditionally compiling ToString override methods under DEBUG preprocessor directives. The change prevents these virtual ToString methods from being included in release builds, improving code trimming efficiency since virtual methods cannot be trimmed by the compiler.

Key changes:

  • Wrap all ToString override methods in #if DEBUG directives across handle types
  • Update the code generator to emit the conditional compilation directives

Reviewed Changes

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

File Description
src/coreclr/tools/Common/Internal/Metadata/NativeFormat/NativeFormatReaderGen.cs Wraps existing ToString overrides in #if DEBUG directives for all handle types
src/coreclr/tools/Common/Internal/Metadata/NativeFormat/Generator/ReaderGen.cs Updates the code generator to emit ToString methods under #if DEBUG

You can also share your feedback on Copilot code review for a chance to win a $100 gift card. Take the survey.

Copy link
Member

@jkotas jkotas left a comment

Choose a reason for hiding this comment

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

I assume that we want to keep GetHashCode/Equals since they are used or to avoid generating fallback support for them.

@MichalStrehovsky
Copy link
Member Author

I assume that we want to keep GetHashCode/Equals since they are used or to avoid generating fallback support for them.

I'm not sure... I can make a PR that replaces them with throws and run CI on it.

Fallback support is generated either way because we don't have analysis for whether someone implemented GetHashCode as base.GetHashCode. (We literally had such code in the BCL during .NET Native times.)

@jkotas jkotas added area-NativeAOT-coreclr and removed needs-area-label An area label is needed to ensure this gets routed to the appropriate area owners labels Aug 14, 2025
Copy link
Contributor

Tagging subscribers to this area: @agocke, @MichalStrehovsky, @jkotas
See info in area-owners.md if you want to be subscribed.

@MichalStrehovsky MichalStrehovsky merged commit c2069df into dotnet:main Aug 15, 2025
91 of 94 checks passed
@MichalStrehovsky MichalStrehovsky deleted the readertostring branch August 15, 2025 05:36
@github-actions github-actions bot locked and limited conversation to collaborators Sep 14, 2025
Sign up for free to subscribe to this conversation on GitHub. Already have an account? Sign in.

Projects

None yet

Development

Successfully merging this pull request may close these issues.

2 participants