-
Notifications
You must be signed in to change notification settings - Fork 12.9k
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
Replace error callback with Result #63286
Conversation
src/librustc_resolve/lib.rs
Outdated
} else { | ||
Ok((path, res)) | ||
match self.resolve_ast_path_inner(&path, is_value) { | ||
Ok(res) => { |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
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.
ebce9f5
to
3cd7f08
Compare
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! |
@bors r+ |
📌 Commit 3cd7f08 has been approved by |
…trochenkov Replace error callback with Result r? @petrochenkov
…trochenkov Replace error callback with Result r? @petrochenkov
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
} | ||
|
||
/// 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( |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
The comment wasn't updated. But other than that I'm really happy the callback is gone!
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
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.
r? @petrochenkov