-
Notifications
You must be signed in to change notification settings - Fork 342
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
Allow multiple [SkipOnCoreClr] #7960
Conversation
cc @safern, @AaronRobinsonMSFT, please take a look. I have not removed |
private static readonly Lazy<bool> s_isGCStress3 = new Lazy<bool>(() => CompareGCStressModeAsLower(GetEnvironmentVariableValue("COMPlus_GCStress"), "0x3", "3") || | ||
CompareGCStressModeAsLower(GetEnvironmentVariableValue("DOTNET_GCStress"), "0x3", "3")); | ||
private static readonly Lazy<bool> s_isGCStressC = new Lazy<bool>(() => CompareGCStressModeAsLower(GetEnvironmentVariableValue("COMPlus_GCStress"), "0xC", "C") || | ||
CompareGCStressModeAsLower(GetEnvironmentVariableValue("DOTNET_GCStress"), "0xC", "C")); |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Maybe we could make GetEnvironmentVariableValue
get the feature name and then that method could consider both prefixes and if any is set return that value? If we do that maybe consider renaming GetEnvironmentVariableValue
to something like GetFeatureEnvironmentVariableValue
.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
It looks much cleaner that way. :)
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Other than one idea, looks great! Thanks 😄
e831917
to
4d8b3bb
Compare
Thanks @am11 -- I'll let you know whenever this is consumed in dotnet/runtime. |
Currently, we can specify multiple conditions with
[SkipOnCoreClr]
that get checked withAND
semantics. To achieveOR
semantics, we need to specify it multiple times (just like[ActiveIssue]
), which is currently not possible. For context, see dotnet/runtime#59564 (comment).In second commit, I have added
DOTNET_
variables for eachCOMPlus_
one. This is because CoreCLR (despite still supportsCOMPlus_
) now considersCOMPlus_
as legacy convention and prefersDOTNET_
. See https://github.com/dotnet/runtime/blob/b0159122f3570decd8ced6228681a210e2711de6/src/coreclr/inc/clrconfignocache.h#L77