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
_______________________________________________
cfe-commits mailing list
cfe-commits@lists.llvm.org
http://lists.llvm.org/cgi-bin/mailman/listinfo/cfe-commits
