[PATCH] D24278: [analyzer] Extend bug reports with extra notes.

[Anna Zaks via cfe-commits]

zaks.anna added a comment.

Thanks!

Looks good overall. Several comments below.


================
Comment at: lib/StaticAnalyzer/Core/HTMLDiagnostics.cpp:160
@@ +159,3 @@
+                    [](const IntrusiveRefCntPtr<PathDiagnosticPiece> &p) {
+                      return isa<PathDiagnosticExtraNotePiece>(p.get());
+                    });
----------------
What do you think about calling these PathDiagnosticNotePiece, specifically, looks like "Extra" does not add anything.

You'll probably need to change quite a few names to remove the "Extra", so up to you.

================
Comment at: lib/StaticAnalyzer/Core/HTMLDiagnostics.cpp:170
@@ +169,3 @@
+      // Will also make a second pass through those later below,
+      // when the header text is ready.
+      HandlePiece(R, FID, **I, NumExtraPieces, TotalExtraPieces);
----------------
Why we need the second path? Please, add a comment as it's not obvious.

================
Comment at: lib/StaticAnalyzer/Frontend/AnalysisConsumer.cpp:119
@@ +118,3 @@
+                                      PE = PD->path.end(); PI != PE; ++PI) {
+        if (!isa<PathDiagnosticExtraNotePiece>(PI->get()))
+          continue;
----------------
a.sidorin wrote:
> if (isa<...>()) {
>   ...
>   ...
> }
> ?
@a.sidorin, LLVM coding guidelines prefer early exits http://llvm.org/docs/CodingStandards.html#use-early-exits-and-continue-to-simplify-code.

================
Comment at: test/Analysis/copypaste/plist-diagnostics.cpp:4
@@ +3,3 @@
+
+void log();
+
----------------
We should call out that this is not working as expected right now. As far as I understand, this file is a test case for a FIXME, correct?

================
Comment at: test/Analysis/copypaste/text-diagnostics.cpp:5
@@ +4,3 @@
+
+int max(int a, int b) { // expected-warning{{Detected code clone}} // expected-note{{Detected code clone}}
+  log();
----------------
It's better to use full sentences for the warning messages: "Clone of this code is detected" (or "Clones of this code are detected" when there are more than one).


================
Comment at: test/Analysis/copypaste/text-diagnostics.cpp:12
@@ +11,3 @@
+
+int maxClone(int a, int b) { // expected-note{{Related code clone is here}}
+  log();
----------------
I would just say: "Code clone here". When I think about it, I assume the very first one is not a clone, but the ones we highlight with notes are clones.


https://reviews.llvm.org/D24278