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

libsyntax: cleanup and refactor pat.rs #63490

Merged
merged 11 commits into from
Aug 15, 2019
Merged

Conversation

Centril
Copy link
Contributor

@Centril Centril commented Aug 12, 2019

A smaller refactoring & cleanup of pat.rs (best read commit by commit).

r? @petrochenkov

@rust-highfive rust-highfive added the S-waiting-on-review Status: Awaiting review from the assignee but also interested parties. label Aug 12, 2019
@Centril
Copy link
Contributor Author

Centril commented Aug 12, 2019

(I started writing up this PR before the note in #63469 (comment) but I'm happy to wait for moving things around.)

@petrochenkov
Copy link
Contributor

I personally don't extract code into functions unless they are called at least twice or take a whole page of text, so I'd rather back out a half of the "extract" commits.
In general, I find it harder to read code if it's broken into small fragments in different places.

@Centril
Copy link
Contributor Author

Centril commented Aug 12, 2019

so I'd rather back out a half of the "extract" commits.

Which ones would you like to keep?

In general, I find it harder to read code if it's broken into small fragments in different places.

Heh; I personally think (and imo this is conventional SE thinking) that large methods forces you to hold more state in your head, that they obscure the overall structure re. the "big picture" (parse_pat_with_range_pat) and "details" (the definitions of the alternatives in the production), and that they violate the single responsibility principle. I think it's a good idea to refactor eagerly (both into more functions and files) and plan for growth; otherwise you tend you grow huge messes like parser.rs was before. Moreover, by extracting into functions, you get the benefits of the internal rustdoc as well as being able to search for functions in there and jump to where you want directly. Debuginfo also gets improved by smaller functions -- instead of being thrown into a large one the backtrace points out a smaller context (up to inlining).

@petrochenkov
Copy link
Contributor

I don't want to argue about this too much, it still looks ok, in particular because every match arm in parse_pat is moved into a separate function (something I didn't notice in commit by commit reading), even if the resulting functions sometimes look like do_this_thing_specifically_while_also_doing_that_thing.

@bors r+

@bors
Copy link
Contributor

bors commented Aug 13, 2019

📌 Commit c8fc4c1 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 Aug 13, 2019
Centril added a commit to Centril/rust that referenced this pull request Aug 14, 2019
…chenkov

libsyntax: cleanup and refactor `pat.rs`

A smaller refactoring & cleanup of `pat.rs` (best read commit by commit).

r? @petrochenkov
Centril added a commit to Centril/rust that referenced this pull request Aug 14, 2019
…chenkov

libsyntax: cleanup and refactor `pat.rs`

A smaller refactoring & cleanup of `pat.rs` (best read commit by commit).

r? @petrochenkov
Centril added a commit to Centril/rust that referenced this pull request Aug 14, 2019
…chenkov

libsyntax: cleanup and refactor `pat.rs`

A smaller refactoring & cleanup of `pat.rs` (best read commit by commit).

r? @petrochenkov
bors added a commit that referenced this pull request Aug 14, 2019
Rollup of 10 pull requests

Successful merges:

 - #62984 (Add lint for excess trailing semicolons)
 - #63075 (Miri: Check that a ptr is aligned and inbounds already when evaluating `*`)
 - #63490 (libsyntax: cleanup and refactor `pat.rs`)
 - #63495 ( Remove redundant `ty` fields from `mir::Constant` and `hair::pattern::PatternRange`.)
 - #63509 (Point at the right enclosing scope when using `await` in non-async fn)
 - #63528 (syntax: Remove `DummyResult::expr_only`)
 - #63534 (Bump to 1.39)
 - #63537 (expand: Unimplement `MutVisitor` on `MacroExpander`)
 - #63542 (Add NodeId for Arm, Field and FieldPat)
 - #63560 (move test that shouldn't be in test/run-pass/)

Failed merges:

r? @ghost
Centril added a commit to Centril/rust that referenced this pull request Aug 14, 2019
…chenkov

libsyntax: cleanup and refactor `pat.rs`

A smaller refactoring & cleanup of `pat.rs` (best read commit by commit).

r? @petrochenkov
Centril added a commit to Centril/rust that referenced this pull request Aug 14, 2019
…chenkov

libsyntax: cleanup and refactor `pat.rs`

A smaller refactoring & cleanup of `pat.rs` (best read commit by commit).

r? @petrochenkov
bors added a commit that referenced this pull request Aug 15, 2019
Rollup of 11 pull requests

Successful merges:

 - #62984 (Add lint for excess trailing semicolons)
 - #63075 (Miri: Check that a ptr is aligned and inbounds already when evaluating `*`)
 - #63490 (libsyntax: cleanup and refactor `pat.rs`)
 - #63507 (When needing type annotations in local bindings, account for impl Trait and closures)
 - #63509 (Point at the right enclosing scope when using `await` in non-async fn)
 - #63528 (syntax: Remove `DummyResult::expr_only`)
 - #63537 (expand: Unimplement `MutVisitor` on `MacroExpander`)
 - #63542 (Add NodeId for Arm, Field and FieldPat)
 - #63543 (Merge Variant and Variant_)
 - #63560 (move test that shouldn't be in test/run-pass/)
 - #63570 (Adjust tracking issues for `MaybeUninit<T>` gates)

Failed merges:

r? @ghost
@bors bors merged commit c8fc4c1 into rust-lang:master Aug 15, 2019
@Centril Centril deleted the cleanup-pat-parser branch August 15, 2019 06:18
@dlrobertson
Copy link
Contributor

🎉

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