Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Remove some unused headers and functions #91177

Merged
merged 16 commits into from
Sep 1, 2023

Conversation

huoyaoyuan
Copy link
Member

No description provided.

@ghost ghost added the community-contribution Indicates that the PR has been added by a community member label Aug 27, 2023
Comment on lines +4 to +6
#ifndef HOST_UNIX
#include <verrsrc.h>
#endif
Copy link
Member Author

Choose a reason for hiding this comment

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

The custom .rc parser on Unix seems honoring #include rule, but does not looks into the actual content of macros. Do we need a comment for it?

Copy link
Member

Choose a reason for hiding this comment

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

If this ifdef is tricky, a comment would be useful.

Copy link
Member

Choose a reason for hiding this comment

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

I am not sure I understand what you mean by "doesn't inspect the actual values of the macros". The parser first runs the resource file through the C preprocessor, so it should process all macros should be expanded to their values during that phase.

Copy link
Member Author

Choose a reason for hiding this comment

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

I didn't look depth about how the parser works and how it works, but it seems that palrt.h doesn't bring any macro used. I'll update the comment.

@huoyaoyuan
Copy link
Member Author

Going to push more today

@huoyaoyuan
Copy link
Member Author

This one is ready for review now. @janvorli any interest?

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.

LGTM. Thank you!

Copy link
Member

@AaronRobinsonMSFT AaronRobinsonMSFT left a comment

Choose a reason for hiding this comment

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

Thank you!

@huoyaoyuan
Copy link
Member Author

Is this fine now?

@AaronRobinsonMSFT AaronRobinsonMSFT merged commit ccb37d5 into dotnet:main Sep 1, 2023
@huoyaoyuan huoyaoyuan deleted the unused branch September 1, 2023 15:55
@ghost ghost locked as resolved and limited conversation to collaborators Oct 1, 2023
Sign up for free to subscribe to this conversation on GitHub. Already have an account? Sign in.
Labels
area-PAL-coreclr community-contribution Indicates that the PR has been added by a community member
Projects
None yet
Development

Successfully merging this pull request may close these issues.

4 participants