-
Notifications
You must be signed in to change notification settings - Fork 1.7k
Fix ICE in upper_case_acronyms
#12903
New issue
Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.
By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.
Already on GitHub? Sign in to your account
Conversation
upper_case_acronyms
and remove most of the string alloc…upper_case_acronyms
for c in s.chars() { | ||
if c.is_ascii_uppercase() { | ||
count += 1; | ||
if count == 3 { |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Why do we check for 3 here (but then 2 in the tail expression)? Doesn't this mean that we won't warn on AB3
or ABc
?
The documentation makes it sound like that we want to emit a warning there
Triggers if there is more than one uppercase char next to each other
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Hmm, the old logic doesn't really seem to follow that very strictly either. The old logic does catch AB3
but not ABc
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Because something like AFoo
shouldn't lint, but AS
should. It really shouldn't check the case for the third letter, but the old logic didn't handle that properly either.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Sorry for taking so long. This looks good to me. I also find this rewrite a lot easier to understand
Not sure if this is ready or you're planning to change something, but r=me if it is
☀️ Test successful - checks-action_dev_test, checks-action_remark_test, checks-action_test |
fixes #12284
The logic has been rewritten to avoid allocations. The old version allocated multiple vecs and strings for each identifier. The new logic allocates a single string only when the lint triggers.
This also no longer lints on strings which don't start with an uppercase letter (e.g.
something_FOO
).changelog: none