Page MenuHomePhabricator

[analyzer] Make issue hash related tests more concise
ClosedPublic

Authored by xazax.hun on Oct 12 2017, 5:45 AM.

Details

Summary

Extend ExprInspection checker to make it possible to dump the issue hash of arbitrary expressions. This change makes it possible to make issue hash related tests more concise and also makes debugging issue hash related problems easier.

Diff Detail

Repository
rL LLVM

Event Timeline

xazax.hun created this revision.Oct 12 2017, 5:45 AM
xazax.hun updated this revision to Diff 118779.Oct 12 2017, 5:52 AM
  • Update the name of the debug function according to review comments

Please, change the commit description to be more comprehensive.

lib/StaticAnalyzer/Checkers/ExprInspectionChecker.cpp
68 ↗(On Diff #118779)

Unrelated change?

78 ↗(On Diff #118779)

I cannot tell what changed here...

xazax.hun added inline comments.Oct 27 2017, 11:22 AM
lib/StaticAnalyzer/Checkers/ExprInspectionChecker.cpp
68 ↗(On Diff #118779)

Since I touched this snippet I reformatted it using clang-format. Apart from adding a new case before the default all other changes are formatting changes. I will revert the formatting changes. So in general, we prefer to minimize the diffs over converging to be clang-formatted?

xazax.hun updated this revision to Diff 120791.Oct 30 2017, 3:29 AM
xazax.hun edited the summary of this revision. (Show Details)
  • Remove formatting changes.
xazax.hun marked 2 inline comments as done.Oct 30 2017, 3:30 AM
NoQ accepted this revision.Oct 30 2017, 4:49 AM

Great stuff!

lib/StaticAnalyzer/Checkers/ExprInspectionChecker.cpp
84 ↗(On Diff #120791)

hashDump <=> DumpHash (looks slightly typo-ish)

288–295 ↗(On Diff #120791)

Accidentally 4 spaces here.

This revision is now accepted and ready to land.Oct 30 2017, 4:49 AM
This revision was automatically updated to reflect the committed changes.

I'd also include some info on how it's now possible to dump the issue hash. You introduce a new debugging function here "clang_analyzer_hashDump" but it's not mentioned in the commit message.

Thanks!

lib/StaticAnalyzer/Checkers/ExprInspectionChecker.cpp
68 ↗(On Diff #118779)

It's much easier to review when the diff does not contain formatting changes intermixed with functional changes. Looks like you can configure clang-format to only format the diff.

xazax.hun added inline comments.Oct 31 2017, 3:29 AM
lib/StaticAnalyzer/Checkers/ExprInspectionChecker.cpp
68 ↗(On Diff #118779)

I see. Unfortunately, formatting only the diff is not always a solution. The clang-format tool cannot format arbitrary source regions, it will extend the formatted region to certain units of the language, in this case to the whole variable definition which spans across multiple lines. Fortunately, it wasn't a big deal to do formatting by hand in this case.

NoQ added a comment.Oct 31 2017, 7:35 AM

Hey, i just recalled that we have documentation for ExprInspection functions in docs/analyzer/DebugChecks.rst, you may want to add your function there as well :)

In D38844#911735, @NoQ wrote:

Hey, i just recalled that we have documentation for ExprInspection functions in docs/analyzer/DebugChecks.rst, you may want to add your function there as well :)

Indeed, thanks for pointing this out! The patch is up for review: https://reviews.llvm.org/D39543