Replace error callback with Result#63286
Merged
bors merged 2 commits intorust-lang:masterfrom Aug 6, 2019
Merged
Conversation
rust-highfive
added
the
S-waiting-on-review
petrochenkov
reviewed
Aug 5, 2019
src/librustc_resolve/lib.rs
Outdated
| } else { | ||
| Ok((path, res)) | ||
| match self.resolve_ast_path_inner(&path, is_value) { | ||
| Ok(res) => { |
Contributor
There was a problem hiding this comment.
Suggested change
| Ok(res) => { | |
| Ok(res) if res != Res::Err => { |
Although, I'd rather remove this condition (and then use ?).
If the function returns Res, then rustdoc should be ready to deal with Res::Err as well.
petrochenkov
added
S-waiting-on-author
Mark-Simulacrum
force-pushed
the
resolve-no-cb
branch
from
ebce9f5 to
3cd7f08
Compare
Member
Author
|
Removed the condition, mostly by uplifting it into rustdoc. I'm not sure if those changes are a correct handling of this -- the code looks a bit questionable to me -- but I don't think this is quite the place to refactor rustdoc's resolution too much :) Happy to do so if you disagree, though! |
Mark-Simulacrum
added
S-waiting-on-review
Contributor
|
@bors r+ |
Collaborator
|
📌 Commit 3cd7f08 has been approved by |
bors
added
S-waiting-on-bors
Centril
added a commit
to Centril/rust
that referenced
this pull request
Aug 6, 2019
…trochenkov Replace error callback with Result r? @petrochenkov
Centril
added a commit
to Centril/rust
that referenced
this pull request
Aug 6, 2019
…trochenkov Replace error callback with Result r? @petrochenkov
bors
added a commit
that referenced
this pull request
Aug 6, 2019
Rollup of 6 pull requests Successful merges: - #62459 (Use internal iteration in the Sum and Product impls of Result and Option) - #62821 (Not listed methods) - #62837 (Fix theme picker blur handler: always hide instead of switching) - #63286 (Replace error callback with Result) - #63296 (Deduplicate rustc_demangle in librustc_codegen_llvm) - #63298 (assume_init: warn about valid != safe) Failed merges: r? @ghost
eddyb
reviewed
Aug 10, 2019
| /// Like `resolve_ast_path`, but takes a callback in case there was an error. | ||
| // FIXME(eddyb) use `Result` or something instead of callbacks. | ||
| fn resolve_ast_path_cb<F>( | ||
| fn resolve_ast_path_inner( |
Member
There was a problem hiding this comment.
The comment wasn't updated. But other than that I'm really happy the callback is gone!
Member
Author
There was a problem hiding this comment.
I think it was lost in a rebase. I've added to my todo list to check if it's been removed after some of the resolver refactorings land so I don't cause merge conflicts and such.
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
5 participants
Add this suggestion to a batch that can be applied as a single commit.This suggestion is invalid because no changes were made to the code.Suggestions cannot be applied while the pull request is closed.Suggestions cannot be applied while viewing a subset of changes.Only one suggestion per line can be applied in a batch.Add this suggestion to a batch that can be applied as a single commit.Applying suggestions on deleted lines is not supported.You must change the existing code in this line in order to create a valid suggestion.Outdated suggestions cannot be applied.This suggestion has been applied or marked resolved.Suggestions cannot be applied from pending reviews.Suggestions cannot be applied on multi-line comments.Suggestions cannot be applied while the pull request is queued to merge.Suggestion cannot be applied right now. Please check back later.
r? @petrochenkov