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.

Sign up for GitHub

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

Jump to bottom

Cleaned up unused CLRConfig code #42422

Merged
merged 3 commits into from
Oct 10, 2020
Merged

Cleaned up unused CLRConfig code #42422

Show file tree
Hide file tree
Changes from all commits
Commits
Show all changes
3 commits
Select commit Hold shift + click to select a range
41a585d
Cleaned up unused configuation retrieval code.
ivdiazsa Sep 17, 2020
71bf639
Merge branch 'master' of https://github.com/ivdiazsa/runtime into spa…
ivdiazsa Sep 29, 2020
65326f1
Restored GetConfigLevel clause effects to preserve original behavior.
ivdiazsa Oct 1, 2020
File filter

Filter by extension

Filter by extension
Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
10 changes: 0 additions & 10 deletions src/coreclr/src/inc/clrconfig.h
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -41,16 +41,6 @@ class CLRConfig
IgnoreEnv = 0x1,
// If set, do not prepend "COMPlus_" when doing environment variable lookup.
DontPrependCOMPlus_ = 0x2,
// If set, don't look in HKLM in the registry.
IgnoreHKLM = 0x4,
// If set, don't look in HKCU in the registry.
IgnoreHKCU = 0x8,
// If set, look only in the system config file, ignoring other config files.
// (This option does not affect environment variable and registry lookups)
ConfigFile_SystemOnly = 0x40,
// If set, reverse the order of config file lookups (application config file first).
// (This option does not affect environment variable and registry lookups)
ConfigFile_ApplicationFirst = 0x80,
// Remove any whitespace at beginning and end of value. (Only applicable for
// *string* configuration values.)
TrimWhiteSpaceFromStringValue = 0x100,
Expand Down
1 change: 0 additions & 1 deletion src/coreclr/src/inc/clrconfigvalues.h
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -780,7 +780,6 @@ CONFIG_DWORD_INFO_DIRECT_ACCESS(INTERNAL_GenerateLongJumpDispatchStubRatio, W("G
CONFIG_DWORD_INFO_EX(INTERNAL_HashStack, W("HashStack"), 0, "", CLRConfig::EEConfig_default)
CONFIG_DWORD_INFO(INTERNAL_HostManagerConfig, W("HostManagerConfig"), (DWORD)-1, "")
CONFIG_DWORD_INFO(INTERNAL_HostTestThreadAbort, W("HostTestThreadAbort"), 0, "")
RETAIL_CONFIG_DWORD_INFO_EX(UNSUPPORTED_IgnoreDllMainReturn, W("IgnoreDllMainReturn"), 0, "Don't check the return value of DllMain if this is set", CLRConfig::ConfigFile_ApplicationFirst)
CONFIG_STRING_INFO(INTERNAL_InvokeHalt, W("InvokeHalt"), "Throws an assert when the given method is invoked through reflection.")
CONFIG_DWORD_INFO_DIRECT_ACCESS(INTERNAL_MaxStackDepth, W("MaxStackDepth"), "")
CONFIG_DWORD_INFO_DIRECT_ACCESS(INTERNAL_MaxStubUnwindInfoSegmentSize, W("MaxStubUnwindInfoSegmentSize"), "")
Expand Down
45 changes: 1 addition & 44 deletions src/coreclr/src/utilcode/clrconfig.cpp
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -82,43 +82,6 @@
#undef CONFIG_STRING_INFO_DIRECT_ACCESS




// Return if a quirk is a enabled.
// This will also return enabled as true when the quirk has a value set.
BOOL CLRConfig::IsConfigEnabled(const ConfigDWORDInfo & info)
{
CONTRACTL
{
NOTHROW;
GC_NOTRIGGER;
FORBID_FAULT;
}
CONTRACTL_END;

DWORD result = info.defaultValue;

//
// Set up REGUTIL options.
//
REGUTIL::CORConfigLevel level = GetConfigLevel(info.options);
BOOL prependCOMPlus = !CheckLookupOption(info, DontPrependCOMPlus_);

REGUTIL::GetConfigDWORD_DontUse_(info.name, info.defaultValue, &result, level, prependCOMPlus);
if(result>0)
return TRUE;
LPWSTR result2 = REGUTIL::GetConfigString_DontUse_(info.name, prependCOMPlus, level);
if(result2 != NULL && result2[0] != 0)
{
return TRUE;
}

if(info.defaultValue>0)
return TRUE;
else
return FALSE;
}

//
// Look up a DWORD config value.
//
Expand Down Expand Up @@ -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 •
edited
Loading

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 •
edited
Loading

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.

level = static_cast<REGUTIL::CORConfigLevel>(level | REGUTIL::COR_CONFIG_USER);

if(CheckLookupOption(options, IgnoreHKLM) == FALSE)
level = static_cast<REGUTIL::CORConfigLevel>(level | REGUTIL::COR_CONFIG_MACHINE);

return level;
return static_cast<REGUTIL::CORConfigLevel>(level | REGUTIL::COR_CONFIG_USER | REGUTIL::COR_CONFIG_MACHINE);
}