-
Notifications
You must be signed in to change notification settings - Fork 13k
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
Add a compiletest header for edition #51800
Conversation
The job Click to expand the log.
I'm a bot! I can only do what humans tell me to, so if this was not helpful or you have suggestions for improvements, please ping or otherwise contact |
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 it'd be better only to supply the --edition
argument if edition:foo
appears in the test. Otherwise, looks great!
Also, I am not sure whether -Zunstable-options
are needed or not =)
src/tools/compiletest/src/header.rs
Outdated
ed @ "2015" | "2018" => Some(ed.to_owned()), | ||
_ => 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.
maybe we should just permit you to write anything here?
src/tools/compiletest/src/header.rs
Outdated
|
||
fn parse_edition(&self, line: &str) -> Option<String> { | ||
match line { | ||
ed @ "2015" | "2018" => Some(ed.to_owned()), |
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.
also, maybe line.trim()
? I don't think we'll have edition names w/ internal whitespace =)
src/tools/compiletest/src/runtest.rs
Outdated
@@ -571,6 +571,10 @@ impl<'test> TestCx<'test> { | |||
|
|||
rustc.args(self.split_maybe_args(&self.config.target_rustcflags)); | |||
rustc.args(&self.props.compile_flags); | |||
rustc.arg(&format!( |
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 would prefer more like
if let Some(e) = self.props.editions {
rustc.arg(&format!("--edition={}", e))
}
this way we can test the compiler's default behavior
src/tools/compiletest/src/runtest.rs
Outdated
@@ -1370,7 +1374,11 @@ impl<'test> TestCx<'test> { | |||
.arg("-o") | |||
.arg(out_dir) | |||
.arg(&self.testpaths.file) | |||
.args(&self.props.compile_flags); | |||
.args(&self.props.compile_flags) | |||
.arg(&format!( |
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.
(same here)
src/tools/compiletest/src/runtest.rs
Outdated
@@ -1765,6 +1773,10 @@ impl<'test> TestCx<'test> { | |||
} | |||
|
|||
rustc.args(&self.props.compile_flags); | |||
rustc.arg(&format!( |
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 here. Maybe we want some helper method to make that nicer.
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 just made this behave as if the user passed compile-flags: --edition=2018
instead.
eb4aba6
to
75d33cf
Compare
@nikomatsakis Ok so...
EDIT: I also squashed everything since the commit history was getting messy. |
@@ -298,6 +298,10 @@ impl TestProps { | |||
.extend(flags.split_whitespace().map(|s| s.to_owned())); | |||
} | |||
|
|||
if let Some(edition) = config.parse_edition(ln) { | |||
self.compile_flags.push(format!("--edition={}", edition)); |
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.
ah, simple. seems good enough for now.
@bors r+ |
📌 Commit 75d33cf has been approved by |
…nikomatsakis Add a compiletest header for edition r? @nikomatsakis Are the `-Zunstable-options` options needed in these tests? It looks like they aren't. If not, I can remove them.
Rollup of 7 pull requests Successful merges: - #49987 (Add str::split_ascii_whitespace.) - #50342 (Document round-off error in `.mod_euc()`-method, see issue #50179) - #51658 (Only do sanity check with debug assertions on) - #51799 (Lower case some feature gate error messages) - #51800 (Add a compiletest header for edition) - #51824 (Fix the error reference for LocalKey::try_with) - #51842 (Document that Layout::from_size_align does not allow align=0) Failed merges: r? @ghost
r? @nikomatsakis
Are the
-Zunstable-options
options needed in these tests? It looks like they aren't. If not, I can remove them.