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

Switch RhConfig from TCHAR to char #86393

Merged
merged 2 commits into from
May 18, 2023

Conversation

elinor-fung
Copy link
Member

@elinor-fung elinor-fung commented May 17, 2023

I didn't see a reason for NativeAOT's RhConfig to be use TCHAR - wchar_t on windows, char otherwise. We can hide away the details of reading the environment variable as wchar vs char and the embedded variables are embedded as ASCII (and read as such) already. This makes it stop using TCHAR for the exposed API which seemed to just be more confusing to consume (this also makes it so that RhConfig.h no longer just assumes some header included before it defines TCHAR/_T).

@ghost
Copy link

ghost commented May 17, 2023

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

Issue Details

I don't think there's any reason for NativeAOT's RhConfig to use TCHAR - defined to be wchar_t on windows, char otherwise. We shouldn't need to read the environment variable as wide characters and the embedded variables are embedded as ASCII (and read as such) already, so having the split seemed to just make it be more confusing to consume and implement (also makes it so that RhConfig.h no longer just assumes some header included before it defines TCHAR/_T)

Author: elinor-fung
Assignees: -
Labels:

area-NativeAOT-coreclr

Milestone: -

@ghost ghost assigned elinor-fung May 17, 2023
Copy link
Contributor

@LakshanF LakshanF left a comment

Choose a reason for hiding this comment

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

Thanks for doing this! I assume we will test this more once EventPipe configuration option is enabled

@@ -1366,17 +1366,8 @@ bool GCToEEInterface::GetBooleanConfigValue(const char* privateKey, const char*
return true;
}
Copy link
Member

Choose a reason for hiding this comment

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

You can also delete TCHAR definition from src\coreclr\gc\env\gcenv.structs.h. I think it was there to make this work.

Copy link
Member Author

Choose a reason for hiding this comment

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

Seems like some other parts of nativeaot are actually using the definition from there now. I might try to detangle in the future change, but I'm going to leave it for now.

@elinor-fung elinor-fung merged commit b25f2bb into dotnet:main May 18, 2023
@elinor-fung elinor-fung deleted the rhconfig-no-tchar branch May 18, 2023 22:35
@ghost ghost locked as resolved and limited conversation to collaborators Jun 18, 2023
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.

3 participants