Two small query improvements#154067
Conversation
It permits four combinations: `false+None`, `false+Some`, `true+None`, and `true+Some`. That final case is not useful; when the bool is true the `Option` is not looked at. This commit changes it to an enum that encodes three combination: `Yes`, `No(None)`, `No(Some)`. I find this easier to read.
It currently uses chaining which is concise but hard to read. There are up to four implicit matches occurring after the call to `execute_query_fn`. - The first `map` on `Option<Erased<Result<T, ErrorGuaranteed>>>`. - The second `map` on `Option<Result<T, ErrorGuaranteed>>`. - The third `map` on `Result<T, ErrorGuaranteed>`. - The `unwrap_or` on `Option<Result<(), ErrorGuaranteed>>`. This commit rewrites it to use at most two matches. - An explicit match on `Option<Erased<Result<T, ErrorGuaranteed>>>`. - A `map` on `Result<T, ErrorGuaranteed>`. This is easier to read. It's also more efficient, though the code isn't hot enough for that to matter.
rustbot
added
A-query-system
|
|
| // We may have created a `DepNode` while determining whether we can skip. | ||
| No(Option<DepNode>), |
There was a problem hiding this comment.
Nit: I'd prefer a named field here (e.g. dep_node), especially since the field's meaning is already non-obvious enough to require a comment.
(In general I think positional enum fields are bad news most of the time, if their meaning isn't overwhelmingly obvious.)
|
Looks good overall, besides the nit. I can imagine some further improvements to |
|
Actually, I'll close this and split it into two separate PRs. #154075 is for the I'll file another one for the |
rustbot
removed
the
S-waiting-on-review
3 participants
Improving a couple of pieces of code I found a little hard to read.
r? @Zalathar