Add this suggestion to a batch that can be applied as a single commit.
This suggestion is invalid because no changes were made to the code.
Suggestions cannot be applied while the pull request is closed.
Suggestions cannot be applied while viewing a subset of changes.
Only one suggestion per line can be applied in a batch.
Add this suggestion to a batch that can be applied as a single commit.
Applying suggestions on deleted lines is not supported.
You must change the existing code in this line in order to create a valid suggestion.
Outdated suggestions cannot be applied.
This suggestion has been applied or marked resolved.
Suggestions cannot be applied from pending reviews.
Suggestions cannot be applied on multi-line comments.
Suggestions cannot be applied while the pull request is queued to merge.
Suggestion cannot be applied right now. Please check back later.
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.
Are there other cases like this? I'm wondering whether we need more cases of handling both the true and false values, and if so, if there's a more general feature needed here, where the linker sees a line like:
and automatically propagates
featurevalue
tovalue
, or something like that.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.
I had proposed this in chat to @eerhardt the other day since it means we are potentially leaving behind bits of "unused" code that are statically known to only be
true
/false
and therefore have no side effects.dotnet/linker#1106 (comment) is a related example where just leaving the
get_IsSupported()
methoods can lead to larger downstream impact due to inheritance chains.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.
In all our existing feature switches, only 1 value will trim any substantial piece of code. For Invariant, I wasn't aware how much unnecessary code was still being rooted for the default setting. I thought only
Invariant=true
would trim a decent amount of code.But I can imagine this being beneficial in other feature switches in the future.
cc @marek-safar @vitek-karas @sbomer - thoughts?
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.
A related bit is that even needing to hardcode
get_Invariant
and others seems potentially unnecessary in certain casesOne would expect the linker could recognize the general pattern of
static readonly bool s_name = AppContext.TryGetSwitch("", out bool result) ? result : default
(or others) and that we wouldn't need to hardcode a table of substitutions outside of more complicated scenarios.It, plus knowing that
static readonly <primitive> name = value
can't change after being set seem like generally good things to recognize as they can lead to later downstream optimizations and therefore trimming opportunitiesThere 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.
If it's common we could make syntax which would be able to substitute the value directly from the feature value. Note that even boolean features have 3 values:
true
,false
,not-set
. I assume the "not-set" would mean no substitution (unless thedefaultfeaturevalue
is used).If this is the only place right now, it's probably not worth it though.
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.
That could work for other cases, but it doesn't work for this case:
runtime/src/libraries/System.Private.CoreLib/src/System/Globalization/GlobalizationMode.Unix.cs
Lines 10 to 38 in 8a52f1e