Manual clippy fixes. #480
Manual clippy fixes. #480
Conversation
Previously we parsed `R -> () : X` as having a return type of `() ` -- note the trailing space! This then meant that a check in `ctbuilder.rs` that doesn't add return types if they're `()` failed, which then led to a warning from Clippy. Trimming the whitespace from the return type in the parser fixes this unfortunate chain and silences Clippy.
This is, unusually, explicitly what we're trying to test.
| let k = j + ':'.len_utf8(); | ||
| if k == self.src.len() || !self.src[k..].starts_with(':') { | ||
| return Ok((j, self.src[i..j].to_string())); | ||
| return Ok((j, self.src[i..j].trim().to_string())); |
There was a problem hiding this comment.
Just two things I notice here, but I'm a bit doubtful that either of these observations require changes to the patch?
First the j value returned and the String value returned are somewhat out of sorts in the (usize, String) return value such that j > i + returned_string.len().
The other thing I noticed, is in regard to the set of whitespace allowed by the rust std libraries trim function
in 38e92e9 we added some stuff to lrlex, and avoided the trim function because it allows some funky characters to be used as whitespace.
However it doesn't seem like I ever did the same for lrpar which is currently already using trim() in parse_action. But it seems like a good idea to keep the set of allowed whitespace the same between lrlex and lrpar?
There was a problem hiding this comment.
First the j value returned and the String value returned are somewhat out of sorts in the (usize, String) return value such that j > i + returned_string.len().
Correct.
But it seems like a good idea to keep the set of allowed whitespace the same between lrlex and lrpar?
Yes, you're probably right. I'm not quite sure how one would do that here though, but unicode is not my strong point!
There was a problem hiding this comment.
Okay, as long as you were aware of the first it doesn't seem like anything that could cause any actual issues to me.
The second thing is probably low priority, could be done in a followup patch if either of us or anyone else gets around to it.
It mostly involves copy/pasting the matches_whitespace function from lrlex from 38e92e9 replacing trim calls with trim_matches.
the parse_ws function in yacc/parser.rs I'll note hard codes ASCII whitespace characters using match rather than using is_whitespace so it at least that currently isn't allowing funky whitespace.
2 participants
No description provided.