Small Refactoring #614
Small Refactoring #614
Conversation
BurntSushi
left a comment
BurntSushi
left a comment
There was a problem hiding this comment.
Thanks for submitting this! I agree with some of these changes, but not all of them. If you want to go through and remove the ones I've noted, then I think this pretty much looks good to go. It would also be very helpful if you could format the commit messages in a style that is consistent with this repo. e.g., style: use is_empty instead of Use 'is_empty'.
| /// be beneficial to avoid finding sub-captures. | ||
| /// | ||
| /// In general, this is called once for every call to `replacen`. | ||
| fn no_expansion<'r>(&'r mut self) -> Option<Cow<'r, [u8]>> { |
There was a problem hiding this comment.
Hmmm I'd actually rather have these lifetimes. I don't agree with all flavors of lifetime elision. In particular, it's not necessarily obvious that the returned Cow has a lifetime attached to sefl when the lifetimes are elided.
There was a problem hiding this comment.
I think this comment applies to all changes in this commit except for ClassQuery, which I like.
There was a problem hiding this comment.
I might be wrong, but I think fn class(query: ClassQuery) will trigger a warning once edition lints are enabled. I think it is supposed to be spelled pub fn class(query: ClassQuery<'_>), ie, 2018 edition soft-deprecates lifetime elision outside of & and &mut. Similarly, the best way to spell this fn might be fn no_expansion(&mut self) -> Option<Cow<'_, [u8]>>. The '_ syntax was stablized in 1.26, so I think we can use it.
There was a problem hiding this comment.
Not a huge fan of the '_ syntax either to be honest... I like the fact that lifetime parameters are introduced just like type parameters.
I'll likely give in once regex switches to 2018, but I'd prefer to just leave well enough alone for now. :-)
bench/src/ffi/tcl.rs
Outdated
| self.re.find_at(self.text, self.last_match).map(|(s, e)| { | ||
| self.last_match = e; | ||
| (s, e) | ||
| }) |
There was a problem hiding this comment.
I personally am not a fan of using map combined with mutations on other objects. Maybe revert this change and others like it above?
regex-syntax/src/hir/translate.rs
Outdated
| } | ||
| cls.iter().next().is_none().ok_or_else(|| { | ||
| self.error(ast.span, ErrorKind::EmptyClassNotAllowed) | ||
| })?; |
There was a problem hiding this comment.
This also just reads weird to me. It feels much more natural as an if statement.
src/utf8.rs
Outdated
| } else { | ||
| Some((cp, n)) | ||
| } | ||
| }) |
There was a problem hiding this comment.
I prefer the previous version. Here, you have to process a monadic bind followed by case analysis. Before, it was just case analysis.
|
r? @BurntSushi |
3 participants
No description provided.