prevent assertion in regex load
ClosedPublic

Authored by davschul on Feb 13 2019, 2:02 PM.

Details

Summary

Instead of crashing the complete application with an assert on a malformed
regex use the logging category to inform users when such a regex is found.
This way the check can also be enabled for release builds, and don't have to
run unnecessarily for debug builds

Diff Detail

Lint
Lint Skipped
Unit
Unit Tests Skipped
davschul created this revision.Feb 13 2019, 2:02 PM
Restricted Application added projects: Kate, Frameworks. · View Herald TranscriptFeb 13 2019, 2:02 PM
Restricted Application added subscribers: kde-frameworks-devel, kwrite-devel. · View Herald Transcript
davschul requested review of this revision.Feb 13 2019, 2:02 PM

I think that makes a lot of sense. Maybe we should even always print the warning, not only in debug mode? See comment below...

PS: When submitting patches without phabricator's arc tool, could you add more diff context, like git diff -U99 .... Then reviewing is much more simple.

src/lib/rule.cpp
579–582 ↗(On Diff #51588)

I even wonder whether this should be always printed, i.e.:

const bool isValid = m_regexp.isValid();
if (!isValid)
    qCWarning(...) << "Invalid regexp:" << m_regexp.pattern();
}
return isValid && !m_regexp.pattern().isEmpty();
davschul added inline comments.Feb 14 2019, 6:41 AM
src/lib/rule.cpp
579–582 ↗(On Diff #51588)

Unconditionally running the isValid check is to expensive. At least according to the comment above the code, but I'haven't checked this statement.

davschul updated this revision to Diff 51643.Feb 14 2019, 6:44 AM

added additional context to the diff

cullmann accepted this revision.Feb 14 2019, 7:05 AM
cullmann added a subscriber: cullmann.

I assume Volker profiled it.
I think it is ok to just check if the logging is on.
99% of the syntax definitions are anyways bundled and are checked by the indexer for such obvious faults.

This revision is now accepted and ready to land.Feb 14 2019, 7:05 AM

I can push that for you, which user to use? "David Schulz" <paste your mail?>

David Schulz david.schulz@qt.io

This revision was automatically updated to reflect the committed changes.