-
Notifications
You must be signed in to change notification settings - Fork 443
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
Conditional Codegen for Messages, Constructors and Events #1458
Conversation
Just make cure it builds with the feature enabled |
Codecov Report
@@ Coverage Diff @@
## master #1458 +/- ##
==========================================
+ Coverage 64.39% 70.12% +5.72%
==========================================
Files 200 200
Lines 6115 6133 +18
==========================================
+ Hits 3938 4301 +363
+ Misses 2177 1832 -345
📣 We’re building smart automated test selection to slash your CI/CD build times. Learn more |
Checked. It works |
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 believe this does not work
/// ``` | ||
/// The function would would iterate over the `cfg` attributes | ||
/// and return `true` if any of the feature flags are set | ||
pub fn is_code_span_enabled(attrs: &[Attribute]) -> bool { |
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 more natural place for this would be ir/utils.rs
for attr_tokens in cfg_attrs { | ||
let _s = attr_tokens.to_token_stream(); | ||
if cfg!(_s) { | ||
return true |
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 believe this logic is wrong in the case of multiple cfg
attributes e.g.
fn main() {
foo_bar();
}
#[cfg(feature = "foo")]
#[cfg(feature = "bar")]
fn foo_bar() {}
cargo build --features foo
-> 🔴
cargo build --features bar
-> 🔴
cargo build --features foo,bar
-> 🟢
So having separate cfg attrs seems to be the equivalent of #[cfg(all(feature = "foo", feature = "bar"))]
The implementation here is instead #[cfg(any(feature = "foo", feature = "bar"))]
.
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.
The logic would be easier to read when expressed e.g. in the form:
pub fn is_code_span_enabled(attrs: &[Attribute]) -> bool {
attrs
.iter()
.filter(|a| a.path.is_ident("cfg"))
.all(|a| {
let predicate = &a.tokens.to_token_stream();
cfg!(predicate)
})
}
} | ||
for attr_tokens in cfg_attrs { | ||
let _s = attr_tokens.to_token_stream(); | ||
if cfg!(_s) { |
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.
Does this even work with the token stream? cfg!
is a compile time macro.
I've tried it and it doesn't appear to work. Even with the literal cfg!(feature = "foo")
it doesn't work for some reason. cfg!(unix)
literal does work.
I think we need to rethink the approach here, maybe see whether we can carry those attributes through to the generated code (I know it is everywhere)
)" This reverts commit 06e9cf2.
This PR closes #1231
It allows constructors, messages and events to be conditionally compiled (i.e. have
#[cfg(...)]
attribute)Example