Skip to content
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

Use ? Kleene operator again #56357

Closed
wants to merge 3 commits into from
Closed

Conversation

mark-i-m
Copy link
Member

In #51587 I removed a bunch of uses of the ? Kleene operator. Now ? is stable, so I am adding them back 🎉

Unfortunately, the macros had all changed slightly since that PR. In particular, a few of them have extra arms, which I also tried to apply ? to. I think I got it right, but I would appreciate an extra careful review...

@rust-highfive
Copy link
Collaborator

r? @davidtwco

(rust_highfive has picked a reviewer for you, use r? to override)

@rust-highfive rust-highfive added the S-waiting-on-review Status: Awaiting review from the assignee but also interested parties. label Nov 29, 2018
@mark-i-m
Copy link
Member Author

I am getting some test failures locally... are we not using rust 2018 within the compiler?

@rust-highfive

This comment has been minimized.

@davidtwco
Copy link
Member

I am getting some test failures locally... are we not using rust 2018 within the compiler?

I'm not sure about this, not something I've ever run into. Could you fix the tidy issue so that we can see if the error you're seeing shows up on Travis?

@petrochenkov
Copy link
Contributor

The compiler is certainly not using 2018 edition.

@petrochenkov
Copy link
Contributor

I still think ? should be supported on both edition.

@mark-i-m
Copy link
Member Author

I still think ? should be supported on both edition.

That ship has sailed already...

The compiler is certainly not using 2018 edition.

Interesting... Is that just a matter of running rustfmt and updating x.py? Are there plans to ever do that?

Could you fix the tidy issue so that we can see if the error you're seeing shows up on Travis?

Sure, when I get to a computer

@mark-i-m
Copy link
Member Author

@davidtwco I pushed a tidy fix, but if the compiler is not using rust 2018, then this PR cannot be merged... ? requires 2018.

@rust-highfive
Copy link
Collaborator

The job x86_64-gnu-llvm-5.0 of your PR failed on Travis (raw log). Through arcane magic we have determined that the following fragments from the build log may contain information about the problem.

Click to expand the log.
travis_time:end:10aabe59:start=1543608385465743482,finish=1543608439889751737,duration=54424008255
$ git checkout -qf FETCH_HEAD
travis_fold:end:git.checkout

Encrypted environment variables have been removed for security reasons.
See https://docs.travis-ci.com/user/pull-requests/#Pull-Requests-and-Security-Restrictions
$ export SCCACHE_BUCKET=rust-lang-ci-sccache2
$ export SCCACHE_REGION=us-west-1
Setting environment variables from .travis.yml
$ export IMAGE=x86_64-gnu-llvm-5.0
---
[00:05:40] command did not execute successfully: "/checkout/obj/build/x86_64-unknown-linux-gnu/stage0/bin/cargo" "build" "--target" "x86_64-unknown-linux-gnu" "-j" "4" "--release" "--locked" "--color" "always" "--features" "" "--manifest-path" "/checkout/src/rustc/Cargo.toml" "--message-format" "json"
[00:05:40] expected success, got: exit code: 101
[00:05:40] failed to run: /checkout/obj/build/bootstrap/debug/bootstrap build
[00:05:40] Build completed unsuccessfully in 0:02:20
[00:05:40] Makefile:28: recipe for target 'all' failed
[00:05:40] make: *** [all] Error 1
The command "stamp sh -x -c "$RUN_SCRIPT"" exited with 2.
travis_time:start:1634c3c3
$ date && (curl -fs --head https://google.com | grep ^Date: | sed 's/Date: //g' || true)
Fri Nov 30 20:13:08 UTC 2018

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 @TimNN. (Feature Requests)

@davidtwco
Copy link
Member

@mark-i-m I'm not too sure. I don't know if this has come up before. One way I could think of doing it, though it is admittedly quick hacky, would be to modify the below code so that the 2018 branch is used if cfg!(stage0) || cfg!(stage1) || cfg!(stage2):

match edition {
Edition::Edition2015 => parse_sep_and_kleene_op_2015(
input,
span,
sess,
features,
attrs,
macro_node_id,
),
Edition::Edition2018 => parse_sep_and_kleene_op_2018(input, span, sess, features, attrs),
_ => unimplemented!(),
}

I feel like that's really awful though. Hopefully someone else knows of a better way than modifying the check to identify the compiler in some way.

@mark-i-m
Copy link
Member Author

@davidtwco I could do that, but what I would really like to do is just use rust 2018. Do you know if there is any blocker to just passing --edition 2018 through x.py and running rustfmt?

@davidtwco
Copy link
Member

@mark-i-m I have no idea. There may be good reasons why that hasn't happened, but maybe not. 🤷‍♂️

@mark-i-m
Copy link
Member Author

mark-i-m commented Dec 1, 2018

@davidtwco I played around with this a bit, and it looks like the may challenge is just that rustfix needs to be run on the compiler, which is not something x.py can do at the moment. Another problem is that rustfix PRs are likely to be large and messy and might require some tree-locking...

@davidtwco
Copy link
Member

@mark-i-m Perhaps that it might be best if this lands after Rust 2018 is out in that case. The compiler could switch to 2018, rustfix and rustfmt could be run.

@mark-i-m mark-i-m closed this Dec 1, 2018
@mark-i-m
Copy link
Member Author

mark-i-m commented Dec 1, 2018

@davidtwco This is the diff just to make libsyntax compile with 2018: https://github.com/rust-lang/rust/compare/master...mark-i-m:2018syntax?w=1

@mark-i-m mark-i-m deleted the use-ques-macro branch May 25, 2019 02:17
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
S-waiting-on-review Status: Awaiting review from the assignee but also interested parties.
Projects
None yet
Development

Successfully merging this pull request may close these issues.

None yet

4 participants