de-macro-ize feature gate checking #66945
Conversation
rust-highfive
added
the
S-waiting-on-review
This comment has been minimized.
This comment has been minimized.
Centril
force-pushed
the
de-macro-gating
branch
from
e87c636
to
6b73566
Compare
This comment has been minimized.
This comment has been minimized.
Centril
force-pushed
the
de-macro-gating
branch
from
6b73566
to
0bc0667
Compare
This comment has been minimized.
This comment has been minimized.
|
It would be good to make sure this is not a perf regression, a big match statement is presumably worse than direct field access -- but it might not matter in practice (and maybe LLVM is amazing and optimizes out the match anyway). |
This comment has been minimized.
This comment has been minimized.
|
Awaiting bors try build completion |
This comment has been minimized.
This comment has been minimized.
Centril
added
S-waiting-on-author
Centril
force-pushed
the
de-macro-gating
branch
from
0bc0667
to
359de34
Compare
|
@bors try |
de-macro-ize feature gate checking This PR removes the usage of macros in feature gating preferring to take in symbols instead. The aim is to make feature gating more understandable overall while also reducing some duplication. To check whether a feature gate is active, `tcx.features().on(sym::my_feature)` can be used. Inside `PostExpansionVisitor` it's however better to use `self.gate(...)` which provides a nicer API atop of that. Outside of the visitor, if `gate_feature` can be used, it should be preferred over `feature_err`. As a follow-up, and once #66878 is merged, it might be a good idea to add a method to `Session` for more convenient access to `gate_feature` and `feature_err` respectively. Originally I intended to go for a `HashSet` representation, but this felt like a smaller change to just expand to a `match` on the symbol => field pairs. r? @oli-obk
|
☀️ Try build successful - checks-azure |
|
Queued 19878fe with parent f577b0e, future comparison URL. |
|
Finished benchmarking try commit 19878fe, comparison URL. |
| /// Panics if the symbol doesn't correspond to a declared feature. | ||
| pub fn on(&self, name: Symbol) -> bool { | ||
| match name { | ||
| $(sym::$feature => self.$feature,)+ |
There was a problem hiding this comment.
match is a pretty inefficient tool in this case (in terms of performance).
Simple linear (not even binary) search in an array could be better here.
And hash map would be both simple and fast, see how built-in attributes gating is implemented.
There was a problem hiding this comment.
I wish we had field pointers (like &MyStruct::my_field).
They can be emulated with offset_of, but that would mean macros again.
There was a problem hiding this comment.
Perhaps the field approach can be kept with the use of macros being minimized rather than fully eliminated.
There was a problem hiding this comment.
Initially I had a HashSet but was hoping that the match would turn into a huge lookup table and be better that way. I can certainly try a HashSet again and see if it changes anything.
Otherwise, perhaps field accesses outside check.rs would be workable (ostensibly that's where the regressions are coming from in some hot loop somewhere?) -- that should still leave us with no macros.
There was a problem hiding this comment.
It is unclear to me -- why is the macro itself bad? It looks like this is still using a macro. It looks also like we're still generating the fields, so we why can't expose those is not clear to me.
| /// Is the given feature enabled? | ||
| /// | ||
| /// Panics if the symbol doesn't correspond to a declared feature. | ||
| pub fn on(&self, name: Symbol) -> bool { |
There was a problem hiding this comment.
If the field approach is rejected after all in favor of a method, could it be named enabled instead of on?
"on" looks like it's... an event handler of sorts? onKeyPress anyone?
The "on/off" association certainly didn't come immediately to me.
There was a problem hiding this comment.
Heh; I went with .active(..) first, then changed to .enabled(..) and finally to .on(..) to make it shorter. 😂 .enabled(..) is fine by me.
|
Ping from triage: |
|
Ping from Triage: Any updates @Centril? |
|
Closing this due to inactivity |
Dylan-DPC-zz
added
S-inactive
This PR removes the usage of macros in feature gating preferring to take in symbols instead. The aim is to make feature gating more understandable overall while also reducing some duplication.
To check whether a feature gate is active,
tcx.features().on(sym::my_feature)can be used.Inside
PostExpansionVisitorit's however better to useself.gate(...)which provides a nicer API atop of that. Outside of the visitor, ifgate_featurecan be used, it should be preferred overfeature_err.As a follow-up, and once #66878 is merged, it might be a good idea to add a method to
Sessionfor more convenient access togate_featureandfeature_errrespectively.Originally I intended to go for a
HashSetrepresentation, but this felt like a smaller change to just expand to amatchon the symbol => field pairs.r? @oli-obk