Repository navigation
Conversation
rustbot
added
S-waiting-on-review
|
r? @eholk rustbot has assigned @eholk. Use Why was this reviewer chosen?The reviewer was selected based on:
|
| let mut v: Vec<&str> = v.unwrap_or_default().split(",").collect(); | ||
| v.sort_unstable(); |
There was a problem hiding this comment.
The new code does not reset the slot to an empty Vec for a None argument. I'm curious to see if that matters.
There was a problem hiding this comment.
That would cause -Zoffload= to no longer override -Zoffload=a,b,c and instead silently get igbored, right? That is inconsistent with other cli flags where the last occurence does win.
There was a problem hiding this comment.
That is true, but at the same time, the empty value is the only one that overrides in this way: -Zoffload=a -Zoffload=b is the same as -Zoffload=a,b not as `-Zoffload=b', so it seems to already be inconsistent.
Anyway, no change was intended, so I'll revert. Thanks for explaining the significance to me!
There was a problem hiding this comment.
-Zoffload=a -Zoffload=b is the same as -Zoffload=a,b not as `-Zoffload=b'
Huh, that is inconsistent with other args that use parse_comma_list.
There was a problem hiding this comment.
Yes, the reset only happens for an empty argument.
There was a problem hiding this comment.
The function just before doesn't do a reset ever. Should it?
pub(crate) fn parse_dump_mono_stats(slot: &mut DumpMonoStatsFormat, v: Option<&str>) -> bool {
match v {
None => true, // hkBst: empty arg, no reset <-----
Some("json") => {
*slot = DumpMonoStatsFormat::Json;
true
}
Some("markdown") => {
*slot = DumpMonoStatsFormat::Markdown;
true
}
Some(_) => false,
}
}
| let mut v: Vec<&str> = v.unwrap_or_default().split(",").collect(); | ||
| v.sort_unstable(); | ||
| for &val in v.iter() { | ||
| v.iter().all(|val| { |
There was a problem hiding this comment.
I don't think using side-effects inside of an iterator method is an improvement.
There was a problem hiding this comment.
I agree that that is not ideal, but I think that disadvantage is outweighed by the advantage of showing this is an all combinator instead of some random for loop that just happens to be a manual implementation of the all combinator.
|
☔ The latest upstream changes (presumably #161014) made this pull request unmergeable. Please resolve the merge conflicts by rebasing. |
|
Thanks for the PR. I had a look at the changes and I don't think I agree these are net improvements. For now I will close this PR. If you'd like to discuss about enforcing certain clippy lints on the compiler tree, happy to discuss that on zulip. |
rustbot
removed
the
S-waiting-on-review
|
@jieyouxu thanks for taking a look! The point of this PR is not really to enforce the clippy lints on the code, but rather to benefit from the conciseness that better use of standard library functions affords. Currently that benefit stands at +69/-161, which I think should be an uncontroversial benefit. Of course not at the cost of worse code which can be subjective. Happy to discuss the particulars. I'll see about opening a topic on Zulip. |
No description provided.