Skip to content

Apply T! macro where posible #1278

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

Merged
merged 2 commits into from
May 15, 2019
Merged

Apply T! macro where posible #1278

merged 2 commits into from
May 15, 2019

Conversation

pasa
Copy link
Contributor

@pasa pasa commented May 15, 2019

apply T! macro implemented in #1248

Copy link
Member

@matklad matklad left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Awesome!

This looks so much better now! Let's remove braces from the PR though: T!['}'] does not look exactly right

@@ -49,7 +49,7 @@ impl<N: AstNode> AstEditor<N> {

fn do_make_multiline(&mut self) {
let l_curly =
match self.ast().syntax().children_with_tokens().find(|it| it.kind() == L_CURLY) {
match self.ast().syntax().children_with_tokens().find(|it| it.kind() == T!['{']) {
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Hm, interesting, I've missed that while reviewing the original PR.

Indeed, we need quotes here, because { break token tree structure. The API looks to sigil heavy as a result though.

Let's maybe scale this back, remove support for {, }, (, ), [, ] from T and stick with L_CURLY and friends?

@matklad
Copy link
Member

matklad commented May 15, 2019

Hm, OTOH, T!['{'] is inline with how various parser generators handle things.

Let's just merge this, we can always revert to L_CURLY later :D

bors r+

bors bot added a commit that referenced this pull request May 15, 2019
1278: Apply T! macro where posible r=matklad a=pasa

apply T! macro implemented in #1248

Co-authored-by: Sergey Parilin <sergey.parilin@fxdd.com>
@bors
Copy link
Contributor

bors bot commented May 15, 2019

Build succeeded

@bors bors bot merged commit 993abed into rust-lang:master May 15, 2019
@jens1o
Copy link
Contributor

jens1o commented May 17, 2019

I've got the feeling that this increases compile time a lot. Is this known?

@matklad
Copy link
Member

matklad commented May 17, 2019

Hm, no, I haven't check it, will run some benchmarks, thanks for the report.

If you have already measured the impact of this change on compile time, I'd be interested in seeing the results!

We might also look at the travis build times I guess.

@matklad
Copy link
Member

matklad commented May 17, 2019

Hm, I've checked locally, and compile time seems the same.

Specififivally, I've check f711237
and 64ab5ab by

repeateadly running

$ find ./target/debug -type f -maxdepth 1 -delete && rm -fr ./target/debug/{deps,.fingerprint}/{*ra_*,*test*,*tools*,*gen_lsp*,*thread_worker*}
$ cargo check

and

$ find ./target/debug -type f -maxdepth 1 -delete && rm -fr ./target/debug/{deps,.fingerprint}/{*ra_*,*test*,*tools*,*gen_lsp*,*thread_worker*}
$ cargo test --no-run

in both cases compile itme was more or less the same for both commits

@jens1o could you give a little more details about where you are observing longer compile times?

@jens1o
Copy link
Contributor

jens1o commented May 18, 2019

I digged a little bit further. At some point yesterday, I just quit compiling(I guessed that all the macro resolving took that much time). I now let it run for some while, only to notice that ra_ide_api results into overflow evaluating the requirement (something with salsa). Might be like rust-lang/rust#47032? I do not know how to get rid of it, though. :/ When it really overflows, then this might be the reason why it takes sooo long.

@matklad
Copy link
Member

matklad commented May 18, 2019

Hm, whihc version of rustc are you using?

@matklad
Copy link
Member

matklad commented May 18, 2019

I think this is the same as #1283: on the latest nightly I also get a very long compile time + overflow at the end.

@jens1o
Copy link
Contributor

jens1o commented May 18, 2019

Ah, yes it is! Thank you very much and pardon me!

@matklad
Copy link
Member

matklad commented May 18, 2019

Thanks for the report! Tacking that overflowing error is important: it has been my main gripe with rustc since all hands :D

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

3 participants