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

Cleaned up unused CLRConfig code #42422

Merged
merged 3 commits into from
Oct 10, 2020
Merged

Cleaned up unused CLRConfig code #42422

merged 3 commits into from
Oct 10, 2020

Conversation

ivdiazsa
Copy link
Member

Fixes #40028.

This PR removes values such as IgnoreHKLM, IgnoreHKCU, ConfigFile_SystemOnly, ConfigFile_ApplicationFirst, which were used to read configuration from registry. They are no longer used, and also removed a function that is no longer called in the CoreCLR codebase.

@ghost
Copy link

ghost commented Sep 18, 2020

Tagging subscribers to this area: @maryamariyan
See info in area-owners.md if you want to be subscribed.

@@ -418,11 +381,5 @@ REGUTIL::CORConfigLevel CLRConfig::GetConfigLevel(LookupOptions options)
if(CheckLookupOption(options, IgnoreEnv) == FALSE)
level = static_cast<REGUTIL::CORConfigLevel>(level | REGUTIL::COR_CONFIG_ENV);

if(CheckLookupOption(options, IgnoreHKCU) == FALSE)
Copy link
Member

Choose a reason for hiding this comment

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

Is this going to change behavior? It looks like we have been always reading the config from registry before this change, but it will no longer happen after this change.

It may be a good idea to remove reading of the config from registry for CoreCLR (it is reading it from the same place as .NET Framework that is a problem on its own), but it should be done as intentional change.

Copy link
Member Author

Choose a reason for hiding this comment

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

Do you mean to not remove this clause in this PR? But then wouldn't it fail when building due to IgnoreHKCU not being defined anymore?

Copy link
Member

@jkotas jkotas Sep 28, 2020

Choose a reason for hiding this comment

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

You would need to remove the if condition, but keep the code that executed when the condition was true.

If you would like to delete the config reading from the registry, it would be fine with me too. It would need to be marked as breaking change and the PR should delete all code related to it.

Copy link
Member Author

Choose a reason for hiding this comment

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

By all related code you mean find all calls to CLRConfig::GetConfigLevel where options is equal to any of the removed flags? Another question I have is what does remoting the if condition mean in this context?

Copy link
Member

@jkotas jkotas Sep 28, 2020

Choose a reason for hiding this comment

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

what does remoting the if condition mean in this context?

Sorry, this was typo - fixed above. I meant to say "You would need remove ...".

all related code

I meant delete REGUTIL::COR_CONFIG_USER, REGUTIL::COR_CONFIG_MACHINE and all code that becomes unreachable.

@jkotas jkotas merged commit 9d34507 into dotnet:master Oct 10, 2020
@ghost ghost locked as resolved and limited conversation to collaborators Dec 7, 2020
@ivdiazsa ivdiazsa deleted the spatial_remains branch March 1, 2021 21:51
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.

Cleanup dead code in CLRConfig
4 participants