-
Notifications
You must be signed in to change notification settings - Fork 503
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
Invertible attributes #2393
base: master
Are you sure you want to change the base?
Invertible attributes #2393
Conversation
9dc142a
to
1c69b18
Compare
c36db49
to
148542b
Compare
@casey this is ready for review |
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.
See some initial comments!
src/attribute_set.rs
Outdated
InvertedStatus::Normal | ||
} | ||
} | ||
_ => return None, |
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 think this function should panic if we call it with a non-invertible attribute.
src/recipe.rs
Outdated
}; | ||
|
||
if cfg!(target_os = "linux") { | ||
return !(disabled.linux || disabled.unix) |
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 these unix
checks redundant with the ones below? I notice that the other Unix OS's don't have the same checks.
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 think this maybe all gets easier if we have a System
enum which represents the current system:
enum System {
Windows,
Linux,
Unix,
...
}
impl System {
fn current() -> &'static [System] {
if cfg(target_os = "linux") {
&[Self::Linux, Self::Unix]
}
}
}
And then we can handle all these checks in the same way.
src/recipe.rs
Outdated
|| (cfg!(target_os = "windows") && windows) | ||
|| (cfg!(unix) && unix) | ||
|| (cfg!(windows) && windows) | ||
use attribute_set::InvertedStatus; |
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'm not sure I like introducing inverted status only to turn it back to a bool with matches!
. I think it would probably be better to just use a bool the whole way.
src/attribute.rs
Outdated
@@ -13,17 +13,17 @@ pub(crate) enum Attribute<'src> { | |||
Doc(Option<StringLiteral<'src>>), | |||
Extension(StringLiteral<'src>), | |||
Group(StringLiteral<'src>), | |||
Linux, | |||
Macos, | |||
Linux { inverted: 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.
I think we should probably use:
Linux { inverted: bool }, | |
Linux { enabled: bool }, |
And invert the meaning of the boolean, to make the downstream code make more sense.
src/attribute.rs
Outdated
AttributeDiscriminant::Private => Self::Private, | ||
AttributeDiscriminant::Script => Self::Script({ | ||
Ok(match (inverted, discriminant) { | ||
(inverted, AttributeDiscriminant::Linux) => Self::Linux { inverted }, |
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.
(inverted, AttributeDiscriminant::Linux) => Self::Linux { inverted }, | |
(inverted, AttributeDiscriminant::Linux) => Self::Linux { enabled: !inverted }, |
@@ -1135,7 +1135,18 @@ impl<'run, 'src> Parser<'run, 'src> { | |||
token.get_or_insert(bracket); | |||
|
|||
loop { | |||
let name = self.parse_name()?; | |||
let (name, inverted) = { |
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 think this is probably cleaner than using mutable variables:
let (name, inverted) = { | |
let (name, inverted) = { | |
let name = self.parse_name()?; | |
if name.lexeme() == "not" { | |
self.expect(ParenL)?; | |
let name = self.parse_name()?; | |
(name, true) | |
} else { | |
(name, false) | |
} | |
} |
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.
And I think we should actually, since we don't have any invertable variables which aren't system variables, change the argument to Attribute::new
to be !inverted, and within Attribute::new
it's called enabled
.
7d30976
to
78c67e2
Compare
b806a9f
to
c3f4feb
Compare
c3f4feb
to
1bacd8c
Compare
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.
See comments. This looks great, I like the structure, but we definitely need to do some kind of more extensive testing.
if cfg!(unix) { | ||
return Unix; | ||
} | ||
panic!("No recognized system"); |
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 don't think this can panic. We should have an Unrecognized
variant.
panic!("No recognized system"); | ||
} | ||
|
||
fn enabled(self, enabled: SystemMap, disabled: SystemMap) -> 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.
I think we need some kind of more extensive testing for this. It looks right to me, but it strikes me as very fragile, and easy to get wrong when extending..
&& (enabled.windows | ||
|| !(enabled.macos || enabled.linux || enabled.openbsd || enabled.unix)) | ||
} | ||
System::MacOS => { |
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.
On MacOS, and other unix systems, a recipe with [not(unix)]
will be enabled. This seems wrong to me.
This PR implements a
not()
syntax for certain OS-target attributes.Resolves #1895