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.

Sign up for GitHub

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

Jump to bottom

Factor out error reporting from smart_resolve_path_fragment fn #58065

Merged
merged 5 commits into from
Feb 10, 2019
Merged

Factor out error reporting from smart_resolve_path_fragment fn #58065

merged 5 commits into from
Feb 10, 2019

Conversation

alexreg
Copy link
Contributor

@alexreg alexreg commented Feb 2, 2019 •
edited
Loading

This function was ridiculously monolithic before. We now have three rather-less-monolithic-and-horrifying functions.

r? @Centril

@rust-highfive rust-highfive added the S-waiting-on-review Status: Awaiting review from the assignee but also interested parties. label Feb 2, 2019
@alexreg alexreg force-pushed the refactor-smart_resolve_path_fragment branch from 72bdf5b to 9fb908a Compare February 2, 2019 01:10
Centril
Centril reviewed Feb 2, 2019
View reviewed changes
Copy link
Contributor

@Centril Centril left a comment

Choose a reason for hiding this comment

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

Some potential places for refactoring... Not sure if the result will be better without looking at all dependencies, but just from the code structure, that's where I'd break into new functions.

src/librustc_resolve/lib.rs Outdated Show resolved Hide resolved
src/librustc_resolve/lib.rs Outdated Show resolved Hide resolved
src/librustc_resolve/lib.rs Outdated Show resolved Hide resolved
src/librustc_resolve/lib.rs Outdated Show resolved Hide resolved
src/librustc_resolve/lib.rs Outdated Show resolved Hide resolved
src/librustc_resolve/lib.rs Outdated Show resolved Hide resolved
src/librustc_resolve/lib.rs Outdated Show resolved Hide resolved
src/librustc_resolve/lib.rs Outdated Show resolved Hide resolved
src/librustc_resolve/lib.rs Outdated Show resolved Hide resolved
@rust-highfive

This comment has been minimized.

Sign in to view
@Centril
Copy link
Contributor

Centril commented Feb 2, 2019

I only looked briefly at the structure... I'll leave the actual review for r? @petrochenkov.

@rust-highfive rust-highfive assigned petrochenkov and unassigned Centril Feb 2, 2019
@alexreg alexreg force-pushed the refactor-smart_resolve_path_fragment branch from 9fb908a to 33521ca Compare February 2, 2019 02:20
@alexreg
Copy link
Contributor Author

alexreg commented Feb 2, 2019

@petrochenkov I've done a more moderate amount of refactoring than suggested by @Centril, but take a look, and let me know what you think.

@rust-highfive

This comment has been minimized.

Sign in to view
@alexreg alexreg force-pushed the refactor-smart_resolve_path_fragment branch from 33521ca to 2e82ea1 Compare February 2, 2019 03:10
@petrochenkov
Copy link
Contributor

LGTM.
I'd prefer not to split the functions further.

Could you also move the factored out functions into librustc_resolve/error_reporting.rs?

@petrochenkov petrochenkov added S-waiting-on-author Status: This is awaiting some action (such as code changes or more information) from the author. and removed S-waiting-on-review Status: Awaiting review from the assignee but also interested parties. labels Feb 3, 2019
@alexreg
Copy link
Contributor Author

alexreg commented Feb 3, 2019

@petrochenkov Yeah I agree, further splitting is probably not optimal. We can even leave it at the double split (first commit only) if you prefer, but I'm happy with how it is now.

Points of review addressed.

@alexreg alexreg added S-waiting-on-review Status: Awaiting review from the assignee but also interested parties. and removed S-waiting-on-author Status: This is awaiting some action (such as code changes or more information) from the author. labels Feb 3, 2019
@rust-highfive

This comment has been minimized.

Sign in to view
@petrochenkov petrochenkov added S-waiting-on-author Status: This is awaiting some action (such as code changes or more information) from the author. and removed S-waiting-on-review Status: Awaiting review from the assignee but also interested parties. labels Feb 4, 2019
@alexreg alexreg force-pushed the refactor-smart_resolve_path_fragment branch from a8f15c3 to 29ff3d4 Compare February 4, 2019 10:25
@rust-highfive

This comment has been minimized.

Sign in to view
@alexreg alexreg force-pushed the refactor-smart_resolve_path_fragment branch from 29ff3d4 to 92f1e9b Compare February 4, 2019 13:29
@rust-highfive

This comment has been minimized.

Sign in to view
@alexreg alexreg force-pushed the refactor-smart_resolve_path_fragment branch from 92f1e9b to ea08c19 Compare February 4, 2019 14:11
@alexreg alexreg added S-waiting-on-review Status: Awaiting review from the assignee but also interested parties. and removed S-waiting-on-author Status: This is awaiting some action (such as code changes or more information) from the author. labels Feb 4, 2019
@petrochenkov
Copy link
Contributor

@bors r+

@bors
Copy link
Contributor

bors commented Feb 5, 2019

📌 Commit ea08c194cd1879da577c37871a0a6da58fe3c2b1 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 Feb 5, 2019
@alexreg
Copy link
Contributor Author

alexreg commented Feb 6, 2019

@Centril Definitely in need of one of your roll-up specials soon! ;-)

@bors
Copy link
Contributor

bors commented Feb 7, 2019

☔ The latest upstream changes (presumably #58254) made this pull request unmergeable. Please resolve the merge conflicts.

@bors bors added S-waiting-on-author Status: This is awaiting some action (such as code changes or more information) from the author. and removed S-waiting-on-bors Status: Waiting on bors to run and complete tests. Bors will change the label on completion. labels Feb 7, 2019
@alexreg alexreg force-pushed the refactor-smart_resolve_path_fragment branch from ea08c19 to d5b514d Compare February 7, 2019 17:11
@rust-highfive

This comment has been minimized.

Sign in to view
@alexreg alexreg force-pushed the refactor-smart_resolve_path_fragment branch from d5b514d to fd70e8e Compare February 7, 2019 21:28
@petrochenkov
Copy link
Contributor

@bors r+

@bors
Copy link
Contributor

bors commented Feb 9, 2019

📌 Commit fd70e8e 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-author Status: This is awaiting some action (such as code changes or more information) from the author. labels Feb 9, 2019
@bors
Copy link
Contributor

bors commented Feb 9, 2019

⌛ Testing commit fd70e8e with merge abcfc3b...

bors added a commit that referenced this pull request Feb 9, 2019
@bors
Auto merge of #58065 - alexreg:refactor-smart_resolve_path_fragment, …
abcfc3b 
…r=petrochenkov

Factor out error reporting from `smart_resolve_path_fragment` fn

This function was ridiculously monolithic before. We now have three rather-less-monolithic-and-horrifying functions.

r? @Centril
@bors
Copy link
Contributor

bors commented Feb 10, 2019

☀️ Test successful - checks-travis, status-appveyor
Approved by: petrochenkov
Pushing abcfc3b to master...

@bors bors added the merged-by-bors This PR was explicitly merged by bors. label Feb 10, 2019
@bors bors merged commit fd70e8e into rust-lang:master Feb 10, 2019
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers

@petrochenkov petrochenkov petrochenkov left review comments

@Centril Centril Centril left review comments

Assignees

@petrochenkov petrochenkov

Labels
merged-by-bors This PR was explicitly merged by bors. S-waiting-on-bors Status: Waiting on bors to run and complete tests. Bors will change the label on completion.
Projects
None yet
Milestone
No milestone
Development

Successfully merging this pull request may close these issues.
5 participants
@alexreg @rust-highfive @Centril @petrochenkov @bors