This is an archive of the discontinued Mercurial Phabricator instance.

Differential D6765

rustfilepatterns: shorter code for concatenating slices
ClosedPublic

Authored by valentin.gatienbaron on Aug 26 2019, 12:16 AM.

Details

Reviewers
Alphare
kevincox
Group Reviewers
hg-reviewers
Commits
rHG69195b6f8f97: rustfilepatterns: shorter code for concatenating slices
rHG406bd21d363b: rustfilepatterns: shorter code for concatenating slices

Diff Detail

Repository
rHG Mercurial
Lint
Automatic diff as part of commit; lint not applicable.
Unit
Automatic diff as part of commit; unit tests not applicable.

Event Timeline

Alphare accepted this revision.Aug 26 2019, 2:38 AM
kevincox accepted this revision.Aug 28 2019, 5:26 AM
kevincox added inline comments.
rust/hg-core/src/filepatterns.rs
162

I don't think you need the &_[..]. https://rust.godbolt.org/z/Wo-vza

valentin.gatienbaron added inline comments.Aug 28 2019, 7:44 AM
rust/hg-core/src/filepatterns.rs
162

Indeed, although it took a compiler upgrade.
I don't really understand why the compiler accepts the change here but not in the other places. It seems a bit random to remove the borrow only here. What do you think about the next commit? It would remove the &_[..] more predictably.

kevincox added inline comments.Aug 28 2019, 7:48 AM
rust/hg-core/src/filepatterns.rs
162

For the other ones I think the problem is that it gets the element type from the first element as &Vec<u8>. I think a better approach then creating the wrapper function is doing [vec.as_slice(), b"foobar"].concat().

valentin.gatienbaron marked 2 inline comments as done.Aug 28 2019, 7:59 AM
valentin.gatienbaron added inline comments.
rust/hg-core/src/filepatterns.rs
162

I see. So the reason why the code above was accepted is because pattern was already a &[u8], and not a &Vec<u8>. Thanks!

valentin.gatienbaron marked an inline comment as done.Aug 28 2019, 8:00 AM
valentin.gatienbaron added a commit: rHG406bd21d363b: rustfilepatterns: shorter code for concatenating slices.Sep 5 2019, 3:16 PM
This revision was not accepted when it landed; it landed in state Needs Review.
Closed by commit rHG406bd21d363b: rustfilepatterns: shorter code for concatenating slices (authored by valentin.gatienbaron). · Explain Why
This revision was automatically updated to reflect the committed changes.

Revision Contents
Changeset List

PathPackages
Mrust/hg-core/src/filepatterns.rs (26 lines)

Diff 16385

rust/hg-core/src/filepatterns.rs

return vec![]; return vec![];
} }
match syntax { match syntax {
PatternSyntax::Regexp => pattern.to_owned(), PatternSyntax::Regexp => pattern.to_owned(),
PatternSyntax::RelRegexp => { PatternSyntax::RelRegexp => {
if pattern[0] == b'^' { if pattern[0] == b'^' {
return pattern.to_owned(); return pattern.to_owned();
} }
let mut res = b".*".to_vec(); [b".*", pattern].concat()
kevincoxUnsubmitted
Done

I don't think you need the &_[..]. https://rust.godbolt.org/z/Wo-vza

kevincox: I don't think you need the `&_[..]`. https://rust.godbolt.org/z/Wo-vza
valentin.gatienbaronAuthorUnsubmitted
Done

Indeed, although it took a compiler upgrade.
I don't really understand why the compiler accepts the change here but not in the other places. It seems a bit random to remove the borrow only here. What do you think about the next commit? It would remove the &_[..] more predictably.

valentin.gatienbaron: Indeed, although it took a compiler upgrade. I don't really understand why the compiler accepts…
kevincoxUnsubmitted
Done

For the other ones I think the problem is that it gets the element type from the first element as &Vec<u8>. I think a better approach then creating the wrapper function is doing [vec.as_slice(), b"foobar"].concat().

kevincox: For the other ones I think the problem is that it gets the element type from the first element…
valentin.gatienbaronAuthorUnsubmitted
Done

I see. So the reason why the code above was accepted is because pattern was already a &[u8], and not a &Vec<u8>. Thanks!

valentin.gatienbaron: I see. So the reason why the code above was accepted is because `pattern` was already a `&[u8]`…
res.extend(pattern);
res
} }
PatternSyntax::Path | PatternSyntax::RelPath => { PatternSyntax::Path | PatternSyntax::RelPath => {
if pattern == b"." { if pattern == b"." {
return vec![]; return vec![];
} }
let mut pattern = escape_pattern(pattern); [escape_pattern(pattern).as_slice(), b"(?:/|$)"].concat()
pattern.extend(b"(?:/|$)");
pattern
} }
PatternSyntax::RootFiles => { PatternSyntax::RootFiles => {
let mut res = if pattern == b"." { let mut res = if pattern == b"." {
vec![] vec![]
} else { } else {
// Pattern is a directory name. // Pattern is a directory name.
let mut as_vec: Vec<u8> = escape_pattern(pattern); [escape_pattern(pattern).as_slice(), b"/"].concat()
as_vec.push(b'/');
as_vec
}; };
// Anything after the pattern must be a non-directory. // Anything after the pattern must be a non-directory.
res.extend(b"[^/]+$"); res.extend(b"[^/]+$");
res res
} }
PatternSyntax::RelGlob => { PatternSyntax::RelGlob => {
let mut res: Vec<u8> = vec![];
let glob_re = glob_to_re(pattern); let glob_re = glob_to_re(pattern);
if let Some(rest) = glob_re.drop_prefix(b"[^/]*") { if let Some(rest) = glob_re.drop_prefix(b"[^/]*") {
res.extend(b".*"); [b".*", rest, globsuffix].concat()
res.extend(rest);
} else { } else {
res.extend(b"(?:|.*/)"); [b"(?:|.*/)", glob_re.as_slice(), globsuffix].concat()
res.extend(glob_re);
} }
res.extend(globsuffix.iter());
res
} }
PatternSyntax::Glob PatternSyntax::Glob
| PatternSyntax::RootGlob => { | PatternSyntax::RootGlob => {
let mut res: Vec<u8> = vec![]; [glob_to_re(pattern).as_slice(), globsuffix].concat()
res.extend(glob_to_re(pattern));
res.extend(globsuffix.iter());
res
} }
} }
} }
const GLOB_SPECIAL_CHARACTERS: [u8; 7] = const GLOB_SPECIAL_CHARACTERS: [u8; 7] =
[b'*', b'?', b'[', b']', b'{', b'}', b'\\']; [b'*', b'?', b'[', b']', b'{', b'}', b'\\'];
/// Wrapper function to `_build_single_regex` that short-circuits 'exact' globs /// Wrapper function to `_build_single_regex` that short-circuits 'exact' globs