rustc_metadata: minor tidying#131743
Conversation
This was added in cc3c8bb when it was closer to the `extract_one` call. Move it back near that call.
rustbot
added
S-waiting-on-review
| || file.starts_with(self.target.dll_prefix.as_ref()) | ||
| && file.ends_with(self.target.dll_suffix.as_ref()) | ||
| { | ||
| match file.starts_with("lib").then_some(()).and_then(|()| { |
There was a problem hiding this comment.
? isn't this just .then
There was a problem hiding this comment.
No, because the closure still needs to return Option<T> rather than T.
But this can be .then(...).flatten(). Making that change causes rustfmt to increase indentation, so I've left it as is.
There was a problem hiding this comment.
To be honest I find this .then_some(()).and_then(|()| { confusing. It looks like this is trying to use the scrutinee closure to select an FxIndexMap to insert the entry into. I find this very convoluted. Can't we just do something straightforward, like jieyouxu@6eba971?
There was a problem hiding this comment.
Thanks for the suggestion. I was trying to avoid repeating conditions (your code repeats file_name.starts_with("lib") twice).
I replaced this with a more imperative closure which should be easier to understand. Can you take a look?
There was a problem hiding this comment.
Frankly, I don't think it's worth avoiding the trivial repetitions if that is what's required, I still find this hard to follow than just doing the simple thing. I let other compiler reviewers decide what they find more readable.
There was a problem hiding this comment.
Would you mind removing the waiting-on-author label?
There was a problem hiding this comment.
Would you mind removing the waiting-on-author label?
You can write @rustbot ready to indicate that. cf. https://rustc-dev-guide.rust-lang.org/contributing.html#waiting-for-reviews
|
@rustbot author |
rustbot
added
S-waiting-on-author
tamird
force-pushed
the
find_commandline_library-tidy
branch
from
dd7b38f to
7bacfb2
Compare
|
@rustbot ready |
rustbot
added
S-waiting-on-review
| // FnMut cannot return reference to captured value, so references | ||
| // must be taken outside the closure. | ||
| let rlibs = &mut rlibs; | ||
| let rmetas = &mut rmetas; | ||
| let dylibs = &mut dylibs; | ||
| match (|| { | ||
| if file.starts_with("lib") { | ||
| if file.ends_with(".rlib") { | ||
| return Some(rlibs); | ||
| } | ||
| if file.ends_with(".rmeta") { | ||
| return Some(rmetas); | ||
| } | ||
| } | ||
| let dll_prefix = self.target.dll_prefix.as_ref(); | ||
| let dll_suffix = self.target.dll_suffix.as_ref(); | ||
| if file.starts_with(dll_prefix) && file.ends_with(dll_suffix) { | ||
| return Some(dylibs); | ||
| } | ||
| None | ||
| })() { | ||
| Some(dst) => { | ||
| dst.insert(loc_canon.clone(), PathKind::ExternFlag); | ||
| } | ||
| None => { | ||
| self.crate_rejections | ||
| .via_filename | ||
| .push(CrateMismatch { path: loc_orig.clone(), got: String::new() }); | ||
| } |
There was a problem hiding this comment.
I am personally not a fan of matching on a closure, maybe something like this?
'accept_crate: {
if file.starts_with("lib") {
if file.ends_with(".rlib") {
rlibs.insert(loc_canon.clone(), PathKind::ExternFlag);
break 'accept_crate;
}
if file.ends_with(".rmeta") {
rmetas.insert(loc_canon.clone(), PathKind::ExternFlag);
break 'accept_crate;
}
}
let dll_prefix = self.target.dll_prefix.as_ref();
let dll_suffix = self.target.dll_suffix.as_ref();
if file.starts_with(dll_prefix) && file.ends_with(dll_suffix) {
dylibs.insert(loc_canon.clone(), PathKind::ExternFlag);
break 'accept_crate;
}
self.crate_rejections
.via_filename
.push(CrateMismatch { path: loc_orig.clone(), got: String::new() });
};There was a problem hiding this comment.
That's fragile because removing any break would produce incorrect logic. I've assigned the result to a local variable, do you like that better?
tamird
force-pushed
the
find_commandline_library-tidy
branch
from
7bacfb2 to
94a2be9
Compare
|
good enough 😁 @bors r+ rollup |
bors
added
S-waiting-on-bors
…dy, r=lcnr rustc_metadata: minor tidying Cleaned up some code while investigating rust-lang#131720. See individual commits.
Rollup of 5 pull requests Successful merges: - rust-lang#130136 (Partially stabilize const_pin) - rust-lang#131654 (Various fixes for Xous) - rust-lang#131743 (rustc_metadata: minor tidying) - rust-lang#131823 (Bump libc to 0.2.161) - rust-lang#131850 (Missing parenthesis) r? `@ghost` `@rustbot` modify labels: rollup
…iaskrgr Rollup of 5 pull requests Successful merges: - rust-lang#131654 (Various fixes for Xous) - rust-lang#131743 (rustc_metadata: minor tidying) - rust-lang#131823 (Bump libc to 0.2.161) - rust-lang#131850 (Missing parenthesis) - rust-lang#131857 (Allow dropping dyn principal) r? `@ghost` `@rustbot` modify labels: rollup
Rollup merge of rust-lang#131743 - tamird:find_commandline_library-tidy, r=lcnr rustc_metadata: minor tidying Cleaned up some code while investigating rust-lang#131720. See individual commits.
6 participants
Cleaned up some code while investigating #131720.
See individual commits.