[analyzer] Extend bug reports with extra notes - CloneChecker

Authored by NoQ on Sep 26 2016, 7:42 AM.



This patch applies the bug reporting technique that is being introduced in D24278 to CloneChecker. Extra notes highlight cloned code sections.

Examples of HTML reports:

I slightly fixed the clone checker diagnostics. In particular, analyzer diagnostics, unlike clang diagnostics, cannot end with a period; and i also capitalized the first letter. Also made slight changes to the suspicious clone structure, because path pieces require more than source ranges to operate (didn't look why though).

Originally D24278 contained this patch, but then it was decided to make a separate review. The original diff is an exact copy of the relevant section of the original diff in D24278, with no review comments addressed.

Diff Detail

NoQ retitled this revision from to [analyzer] Extend bug reports with extra notes - CloneChecker.Sep 26 2016, 7:42 AM
NoQ updated this object.
NoQ added a subscriber: cfe-commits.
NoQ updated this revision to Diff 72521.Sep 26 2016, 10:23 AM

Addressed inline comments on the original review D24278. Rebased on top of the updated D24278. Changed warning messages completely according to Anna's comments in D24278.

v.g.vassilev added inline comments.Sep 26 2016, 11:22 AM
102 ↗(On Diff #72521)

I was thinking whether we shouldn't say code snippet or something that qualifies code, as it is too generic term.

112 ↗(On Diff #72521)

Is that missing an indent?

zaks.anna added inline comments.Sep 27 2016, 11:15 AM
7 ↗(On Diff #72521)

"was" -> "is"?
Do we use past or present elsewhere?

61 ↗(On Diff #72521)

The error message seems too verbose and focused on the implementation rather than user (ex: "suspicious code clone" and "suggestion is based").

Maybe we could say something like this:

  • Did you mean to use 'a'?
  • Similar code snippet here
zaks.anna added inline comments.Sep 28 2016, 10:46 AM
61 ↗(On Diff #72521)


Did you mean to use 'a'?
Similar code snippet here uses 'b'

Did you mean to use 'a' instead of 'b'?
Similar code snippet here

zaks.anna added inline comments.Sep 28 2016, 10:55 AM
8 ↗(On Diff #72521)
  • Duplicate code detected
  • Similar code here
61 ↗(On Diff #72521)

Another refinement

Potential copy-paste error: did you mean to use 'a' instead of 'b'?
Similar code here

zaks.anna added inline comments.Sep 28 2016, 11:11 AM
61 ↗(On Diff #72521)

Potential copy-paste error; did you really mean to use 'b' here?
Similar code using 'a' here
Similar code using 'c' here

NoQ updated this revision to Diff 72991.EditedSep 30 2016, 1:05 AM
  • Update the warning messages.
  • Move a run-away test file to its correct directory.
  • Fix a copy-paste error in initialization of the variable field of a clone pair.
This revision is now accepted and ready to land.Sep 30 2016, 8:50 AM

reverted with r283182

reverted with r283182 as it depends on r283092

This revision was automatically updated to reflect the committed changes.
chapuni added a subscriber: chapuni.Oct 4 2016, 1:43 AM
chapuni added inline comments.

You should update \param here. See also r283106.

Out of curiosity, how was MSVC crash solved?

NoQ added a comment.Oct 4 2016, 1:45 PM

Out of curiosity, how was MSVC crash solved?

It wasn't, unfortunately; phabricator closed this revision by looking at the commit that was already reverted, with a delay (see http://lists.llvm.org/pipermail/cfe-commits/Week-of-Mon-20161003/172501.html for more info on the delay).

I'm planning to systematically split D24278 into smaller chunks, looking closely at the buildbot, in order to see what exactly breaks the MSVC compiler.
That would produce a lot of noise, for which i'm sorry in advance, but i don't seem to have better options(?)

NoQ added inline comments.Oct 4 2016, 1:50 PM

Thanks!! Will do.