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

syntax: consolidate function parsing in item.rs #65362

Merged
merged 1 commit into from
Oct 14, 2019

Conversation

Centril
Copy link
Contributor

@Centril Centril commented Oct 13, 2019

Extracted from #65324.

r? @estebank

@rust-highfive rust-highfive added the S-waiting-on-review Status: Awaiting review from the assignee but also interested parties. label Oct 13, 2019
@petrochenkov
Copy link
Contributor

petrochenkov commented Oct 13, 2019

The downside is that I find it progressively harder to find where the code is.
If previously I needed to just Ctrl+F in parser.rs now you have to either remember what code is where, or do file search in libsyntax directory.

In this case I'd personally keep the code in item.rs because functions are items, and item.rs is the go-to place for item parsing, and item.rs is far from being too large. (Plus move the function-related stuff from parser.rs to item.rs.)

@Centril
Copy link
Contributor Author

Centril commented Oct 13, 2019

If previously I needed to just Ctrl+F in parser.rs now you have to either remember what code is where, or do file search in libsyntax directory.

I recommend just setting up a keyword in Firefox rustc myfun that finds the function; it works quickly for me and I don't need to do a file search. It should also be relatively easy to find something function-parsing related by well... looking at the fun.rs file. :)

and item.rs is far from being too large.

It would be roughly >2100 LOC which is imo too large for any file.

(Plus move the function-related stuff from parser.rs to item.rs.)

So the reason I'm doing this change specifically is to facilitate the merging of parsing various variants of functions (free, extern, impl, and traits) as we've discussed before and then less logic would be exposed from fun.rs.

@petrochenkov
Copy link
Contributor

petrochenkov commented Oct 13, 2019

It would be roughly >2100 LOC which is imo too large for any file.

Ok, my personal limit is ~5000 LOC and tidy enforces 3000 which is a bit too small.
Either way, item.rs fits into it with a margin and I think it's more convenient to keep it together.

It should also be relatively easy to find something function-parsing related by well... looking at the fun.rs file

And to find something enum-parsing related by looking into where - enum.rs? No. That's an inconsistency that bring inconvenience.
(I wouldn't want to achieve consistency by creating enum.rs and every-other-item.rs because that would mean many tiny files, which are also bad mkay.)

@Centril
Copy link
Contributor Author

Centril commented Oct 13, 2019

Either way, item.rs fits into it with a margin and I think it's more convenient to keep it together.

Can we at least keep it in one place inside item.rs? I found it a hindrance for my refactorings to jump between various locations (inside item.rs) when refactoring the function grammars.

@petrochenkov
Copy link
Contributor

Can we at least keep it in one place inside item.rs?

Sure! That would be great.

@Centril Centril changed the title syntax: consolidate function parsing in fun.rs syntax: consolidate function parsing in item.rs Oct 13, 2019
@Centril
Copy link
Contributor Author

Centril commented Oct 13, 2019

r? @petrochenkov Updated.

(I wouldn't want to achieve consistency by creating enum.rs and every-other-item.rs because that would mean many tiny files, which are also bad mkay.)

(Fwiw, I actually wanted to merge all the data type stuff (struct, union, and enum) into a single file because they have relatively similar grammars and are related... also, the module wouldn't be tiny that way.)

@Centril
Copy link
Contributor Author

Centril commented Oct 13, 2019

(Moved one import just a bit to make the diff cleaner...)

@petrochenkov
Copy link
Contributor

Thanks!
@bors r+

@bors
Copy link
Contributor

bors commented Oct 13, 2019

📌 Commit 4a0c487 has been approved by petrochenkov

@bors bors added S-waiting-on-bors Status: Waiting on bors to run and complete tests. Bors will change the label on completion. and removed S-waiting-on-review Status: Awaiting review from the assignee but also interested parties. labels Oct 13, 2019
Centril added a commit to Centril/rust that referenced this pull request Oct 13, 2019
syntax: consolidate function parsing in item.rs

Extracted from rust-lang#65324.

r? @estebank
Centril added a commit to Centril/rust that referenced this pull request Oct 14, 2019
syntax: consolidate function parsing in item.rs

Extracted from rust-lang#65324.

r? @estebank
Centril added a commit to Centril/rust that referenced this pull request Oct 14, 2019
syntax: consolidate function parsing in item.rs

Extracted from rust-lang#65324.

r? @estebank
bors added a commit that referenced this pull request Oct 14, 2019
Rollup of 7 pull requests

Successful merges:

 - #65215 (Add long error explanation for E0697)
 - #65292 (Print lifetimes with backticks)
 - #65362 (syntax: consolidate function parsing in item.rs)
 - #65363 (Remove implicit dependencies on syntax::pprust)
 - #65379 (refactor session::config::build_session_options_and_crate_config)
 - #65392 (Move `Nonterminal::to_tokenstream` to parser & don't rely directly on parser in lowering)
 - #65395 (Add some tests for fixed ICEs)

Failed merges:

r? @ghost
bors added a commit that referenced this pull request Oct 14, 2019
Rollup of 7 pull requests

Successful merges:

 - #65215 (Add long error explanation for E0697)
 - #65292 (Print lifetimes with backticks)
 - #65362 (syntax: consolidate function parsing in item.rs)
 - #65363 (Remove implicit dependencies on syntax::pprust)
 - #65379 (refactor session::config::build_session_options_and_crate_config)
 - #65392 (Move `Nonterminal::to_tokenstream` to parser & don't rely directly on parser in lowering)
 - #65395 (Add some tests for fixed ICEs)

Failed merges:

r? @ghost
@bors bors merged commit 4a0c487 into rust-lang:master Oct 14, 2019
@Centril Centril deleted the extract_fun branch October 14, 2019 14:08
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
S-waiting-on-bors Status: Waiting on bors to run and complete tests. Bors will change the label on completion.
Projects
None yet
Development

Successfully merging this pull request may close these issues.

5 participants