Refactor Windows parse_prefix
#74220
Conversation
rust-highfive
added
the
S-waiting-on-review
tesuji
force-pushed
the
windows-path-com
branch
5 times, most recently
from
ffd7b1f
to
cb36cdb
Compare
There was a problem hiding this comment.
Thanks for the PR!
Personally, I would appreciate it if you would (at least briefly) describe the changes and (importantly) the motivation for those changes in the PR description. Otherwise I always have the feeling I am missing some context. And even for regular contributors like you, without knowing what those changes are actually good for, one might think "why should I even bother reviewing?". A simple "I think this refactor makes the code more robust and/or more readable" or "See commit messages for more details" would be enough.
Anyhow, the change look fine to me overall. I left a couple of inline comments though.
src/libstd/sys/windows/path.rs
Outdated
| ) -> Option<(&'a [u8], &'a [u8])> { | ||
| let idx = path.iter().position(|&x| f(x))?; | ||
| // Remove the path component separator | ||
| // SAFETY: `s[idx + 1..] == s[s.len()..]` |
There was a problem hiding this comment.
That safety comment is confusing IMO. How about something like this?
// SAFETY: `idx + 1` is never larger than `path.len()` and `path[path.len()..]`
// is a valid index
Either way, is the unsafe even worth it here? Might this be in a hot loop? I have no idea, but if it's unlikely this function is performance critical, I'd rather avoid the unsafe code.
There was a problem hiding this comment.
I know it is confusing but I couldn't think of any better comments for it.
Thank you for nice suggestion.
Re unsafe, I was haunted by #73396 and outer unsafe block made me feel too powerful to use unsafe here.
Anyway, Nikita assigned to that issue and it would be resolve by next LLVM update, I will drop the unsafe indexing.
|
I am really appreciate your quality reviews. I rebased the patch and revolved |
|
Thanks again, also for quickly addressing my comments. Way faster than my response time :D @bors r+ rollup=iffy (Let's also try this new rollup command. I don't expect this to fail or anything, but I am always conservative with tagging a PR as rollup.) |
|
📌 Commit 967e5e38c168da2df30ae0fc04709ac016835684 has been approved by |
bors
added
S-waiting-on-bors
|
Hi, I renamed the test function. Also I added a new commit e31898b for reducing unsafe scope in |
|
Thanks for the @bors r+ |
|
📌 Commit e31898b has been approved by |
…bertodt Refactor Windows `parse_prefix` These changes make me feel more readable. See the commit messages for more details.
…bertodt Refactor Windows `parse_prefix` These changes make me feel more readable. See the commit messages for more details.
…arth Rollup of 15 pull requests Successful merges: - rust-lang#71237 (Add Ayu theme to rustdoc) - rust-lang#73720 (Clean up E0704 error explanation) - rust-lang#73866 (Obviate #[allow(improper_ctypes_definitions)]) - rust-lang#73965 (typeck: check for infer before type impls trait) - rust-lang#73986 (add (unchecked) indexing methods to raw (and NonNull) slices) - rust-lang#74173 (Detect tuple struct incorrectly used as struct pat) - rust-lang#74220 (Refactor Windows `parse_prefix`) - rust-lang#74227 (Remove an unwrap in layout computation) - rust-lang#74239 (Update llvm-project to latest origin/rustc/10.0-2020-05-05 commit ) - rust-lang#74257 (don't mark linux kernel module targets as a unix environment) - rust-lang#74270 (typeck: report placeholder type error w/out span) - rust-lang#74296 (Clarify the description for rfind) - rust-lang#74310 (Use `ArrayVec` in `SparseBitSet`.) - rust-lang#74316 (Remove unnecessary type hints from Wake internals) - rust-lang#74324 (Update Clippy) Failed merges: r? @ghost
|
☔ The latest upstream changes (presumably #74330) made this pull request unmergeable. Please resolve the merge conflicts. |
bors
added
S-waiting-on-author
These changes make me feel more readable.
See the commit messages for more details.