Page MenuHomePhabricator

[clang-tidy] Fix more false positives for bugprone-string-integer-assignment
ClosedPublic

Authored by courbet on Mar 14 2019, 5:47 AM.

Diff Detail

Repository
rL LLVM

Event Timeline

courbet created this revision.Mar 14 2019, 5:47 AM
Herald added a project: Restricted Project. · View Herald TranscriptMar 14 2019, 5:47 AM
alexfh added inline comments.Mar 14 2019, 6:25 AM
clang-tools-extra/clang-tidy/bugprone/StringIntegerAssignmentCheck.cpp
50 ↗(On Diff #190608)

No need for the top-level const in the first parameter.

70 ↗(On Diff #190608)

nit: Please capitalize the first word.

115 ↗(On Diff #190608)

I believe you should also check (or assert) that E is not instantiation-dependent before running the evaluator.

courbet marked 3 inline comments as done.Mar 14 2019, 6:31 AM
courbet added inline comments.
clang-tools-extra/clang-tidy/bugprone/StringIntegerAssignmentCheck.cpp
115 ↗(On Diff #190608)

Interesting. AFAICT if I don't check that , I end up warning in the cases when the instantiation does result in a too large constant, which is what we want (the user should add a cast if they want to silence this).

courbet updated this revision to Diff 190610.Mar 14 2019, 6:31 AM

Address review comments.

alexfh added inline comments.Mar 19 2019, 9:52 AM
clang-tools-extra/clang-tidy/bugprone/StringIntegerAssignmentCheck.cpp
115 ↗(On Diff #190608)

IIUC, expression evaluation is just not supposed to be used on instantiation-dependent expressions. I've recently fixed a related crash (https://reviews.llvm.org/rL355401). I guess, there's a similar possibility here.

courbet updated this revision to Diff 191885.Mar 22 2019, 8:24 AM

Ignore template contexts and add a test.

courbet marked 2 inline comments as done.Mar 22 2019, 8:26 AM
courbet added inline comments.
clang-tools-extra/clang-tidy/bugprone/StringIntegerAssignmentCheck.cpp
115 ↗(On Diff #190608)

Thanks for the pointer.

alexfh accepted this revision.Mar 22 2019, 12:04 PM

LG. Thanks for improving the check!

This revision is now accepted and ready to land.Mar 22 2019, 12:04 PM
This revision was automatically updated to reflect the committed changes.
courbet marked an inline comment as done.
Herald added a project: Restricted Project. · View Herald TranscriptMon, Mar 25, 1:16 AM