Skip to content

Conversation

erik-krogh
Copy link
Contributor

The overly-large-range query would produce an extremely big alert message on some regexps from natalie-lang/natalie.
This fixes it.

I capped the range at 50 chars, because that broke no tests in the existing test-suite.

@erik-krogh erik-krogh requested a review from aibaars August 29, 2022 19:07
@erik-krogh erik-krogh added the no-change-note-required This PR does not need a change note label Aug 29, 2022
@erik-krogh erik-krogh marked this pull request as ready for review August 29, 2022 19:07
@erik-krogh erik-krogh requested review from a team as code owners August 29, 2022 19:07
@aibaars
Copy link
Contributor

aibaars commented Aug 29, 2022

Looks good to me. Let's merge this into the 3.7 branch.

Copy link
Contributor

@tausbn tausbn left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Looks reasonable to me. I take it that constructing the long string (equiv) is not an issue, just displaying it.

@@ -238,8 +238,13 @@ module RangePrinter {

/** Gets a char range that is overly large because of `reason`. */
RegExpCharacterRange getABadRange(string reason, int priority) {
result instanceof OverlyWideRange and
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I'm surprised that this instanceof check is needed. Isn't this implied by the inline cast on line 243? 🤔

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

You're right, that check is not needed, but I'm going this merge this PR as is anyway.
The cast was needed in one of the first versions of my fix, but it's not needed with the version I ended up with.

Maybe a QL-for-QL query in the style of ql/superfluous-exists?

@erik-krogh
Copy link
Contributor Author

The JavaScript language tests are failing, that is unrelated and will be fixed here: #9751

I'm merging this PR now, and I'll cherry-pick the commit to a new PR against the 3.7 branch.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
Projects
None yet
Development

Successfully merging this pull request may close these issues.

3 participants