Re: [PATCH] D24278: [analyzer] Extend bug reports with extra notes.
zaks.anna accepted this revision. zaks.anna added a comment. This revision is now accepted and ready to land. I have no further comments. https://reviews.llvm.org/D24278 ___ cfe-commits mailing list cfe-commits@lists.llvm.org http://lists.llvm.org/cgi-bin/mailman/listinfo/cfe-commits
Re: [PATCH] D24278: [analyzer] Extend bug reports with extra notes.
NoQ updated this revision to Diff 72519.
NoQ marked 10 inline comments as done.
NoQ added a comment.
Addressed inline comments. Renamed extra notes to notes.
https://reviews.llvm.org/D24278
Files:
include/clang/StaticAnalyzer/Core/AnalyzerOptions.h
include/clang/StaticAnalyzer/Core/BugReporter/BugReporter.h
include/clang/StaticAnalyzer/Core/BugReporter/PathDiagnostic.h
lib/Rewrite/HTMLRewrite.cpp
lib/StaticAnalyzer/Core/AnalyzerOptions.cpp
lib/StaticAnalyzer/Core/BugReporter.cpp
lib/StaticAnalyzer/Core/HTMLDiagnostics.cpp
lib/StaticAnalyzer/Core/PathDiagnostic.cpp
lib/StaticAnalyzer/Core/PlistDiagnostics.cpp
lib/StaticAnalyzer/Frontend/AnalysisConsumer.cpp
Index: lib/StaticAnalyzer/Frontend/AnalysisConsumer.cpp
===
--- lib/StaticAnalyzer/Frontend/AnalysisConsumer.cpp
+++ lib/StaticAnalyzer/Frontend/AnalysisConsumer.cpp
@@ -113,16 +113,28 @@
Diag.Report(WarnLoc, WarnID) << PD->getShortDescription()
<< PD->path.back()->getRanges();
+ // First, add extra notes, even if paths should not be included.
+ for (const auto : PD->path) {
+if (!isa(Piece.get()))
+ continue;
+
+SourceLocation NoteLoc = Piece->getLocation().asLocation();
+Diag.Report(NoteLoc, NoteID) << Piece->getString()
+ << Piece->getRanges();
+ }
+
if (!IncludePath)
continue;
+ // Then, add the path notes if necessary.
PathPieces FlatPath = PD->path.flatten(/*ShouldFlattenMacros=*/true);
- for (PathPieces::const_iterator PI = FlatPath.begin(),
- PE = FlatPath.end();
- PI != PE; ++PI) {
-SourceLocation NoteLoc = (*PI)->getLocation().asLocation();
-Diag.Report(NoteLoc, NoteID) << (*PI)->getString()
- << (*PI)->getRanges();
+ for (const auto : FlatPath) {
+if (isa(Piece.get()))
+ continue;
+
+SourceLocation NoteLoc = Piece->getLocation().asLocation();
+Diag.Report(NoteLoc, NoteID) << Piece->getString()
+ << Piece->getRanges();
}
}
}
Index: lib/StaticAnalyzer/Core/PlistDiagnostics.cpp
===
--- lib/StaticAnalyzer/Core/PlistDiagnostics.cpp
+++ lib/StaticAnalyzer/Core/PlistDiagnostics.cpp
@@ -281,6 +281,9 @@
ReportMacro(o, cast(P), FM, SM, LangOpts,
indent, depth);
break;
+case PathDiagnosticPiece::Note:
+ // FIXME: Extend the plist format to support those.
+ break;
}
}
Index: lib/StaticAnalyzer/Core/PathDiagnostic.cpp
===
--- lib/StaticAnalyzer/Core/PathDiagnostic.cpp
+++ lib/StaticAnalyzer/Core/PathDiagnostic.cpp
@@ -60,6 +60,7 @@
PathDiagnosticCallPiece::~PathDiagnosticCallPiece() {}
PathDiagnosticControlFlowPiece::~PathDiagnosticControlFlowPiece() {}
PathDiagnosticMacroPiece::~PathDiagnosticMacroPiece() {}
+PathDiagnosticNotePiece::~PathDiagnosticNotePiece() {}
void PathPieces::flattenTo(PathPieces , PathPieces ,
bool ShouldFlattenMacros) const {
@@ -95,6 +96,7 @@
}
case PathDiagnosticPiece::Event:
case PathDiagnosticPiece::ControlFlow:
+case PathDiagnosticPiece::Note:
Current.push_back(Piece);
break;
}
@@ -342,15 +344,16 @@
}
switch (X.getKind()) {
-case clang::ento::PathDiagnosticPiece::ControlFlow:
+case PathDiagnosticPiece::ControlFlow:
return compareControlFlow(cast(X),
cast(Y));
-case clang::ento::PathDiagnosticPiece::Event:
+case PathDiagnosticPiece::Event:
+case PathDiagnosticPiece::Note:
return None;
-case clang::ento::PathDiagnosticPiece::Macro:
+case PathDiagnosticPiece::Macro:
return compareMacro(cast(X),
cast(Y));
-case clang::ento::PathDiagnosticPiece::Call:
+case PathDiagnosticPiece::Call:
return compareCall(cast(X),
cast(Y));
}
@@ -1098,6 +1101,10 @@
ID.Add(**I);
}
+void PathDiagnosticNotePiece::Profile(llvm::FoldingSetNodeID ) const {
+ PathDiagnosticSpotPiece::Profile(ID);
+}
+
void PathDiagnostic::Profile(llvm::FoldingSetNodeID ) const {
ID.Add(getLocation());
ID.AddString(BugType);
Index: lib/StaticAnalyzer/Core/HTMLDiagnostics.cpp
===
--- lib/StaticAnalyzer/Core/HTMLDiagnostics.cpp
+++ lib/StaticAnalyzer/Core/HTMLDiagnostics.cpp
@@ -152,13 +152,30 @@
}
// Process the path.
- unsigned n = path.size();
- unsigned max = n;
-
- for (PathPieces::const_reverse_iterator I = path.rbegin(),
- E = path.rend();
-I != E; ++I, --n)
-HandlePiece(R, FID, **I, n, max);
+ // Maintain the counts of
Re: [PATCH] D24278: [analyzer] Extend bug reports with extra notes.
NoQ updated this revision to Diff 72480.
NoQ added a comment.
This diff is a part of the original diff that contains core changes without
checker changes (in particular, without tests). No changes were made, no
comments addressed, just removed checker stuff. Uploading it separately so that
it was easier to track changes in phabricator.
Actually improved diff coming soon.
Separate reviews for the checkers coming soon.
https://reviews.llvm.org/D24278
Files:
include/clang/StaticAnalyzer/Core/AnalyzerOptions.h
include/clang/StaticAnalyzer/Core/BugReporter/BugReporter.h
include/clang/StaticAnalyzer/Core/BugReporter/PathDiagnostic.h
lib/Rewrite/HTMLRewrite.cpp
lib/StaticAnalyzer/Core/AnalyzerOptions.cpp
lib/StaticAnalyzer/Core/BugReporter.cpp
lib/StaticAnalyzer/Core/HTMLDiagnostics.cpp
lib/StaticAnalyzer/Core/PathDiagnostic.cpp
lib/StaticAnalyzer/Core/PlistDiagnostics.cpp
lib/StaticAnalyzer/Frontend/AnalysisConsumer.cpp
Index: lib/StaticAnalyzer/Frontend/AnalysisConsumer.cpp
===
--- lib/StaticAnalyzer/Frontend/AnalysisConsumer.cpp
+++ lib/StaticAnalyzer/Frontend/AnalysisConsumer.cpp
@@ -113,13 +113,27 @@
Diag.Report(WarnLoc, WarnID) << PD->getShortDescription()
<< PD->path.back()->getRanges();
+ // First, add extra notes, even if paths should not be included.
+ for (PathPieces::const_iterator PI = PD->path.begin(),
+ PE = PD->path.end(); PI != PE; ++PI) {
+if (!isa(PI->get()))
+ continue;
+
+SourceLocation NoteLoc = (*PI)->getLocation().asLocation();
+Diag.Report(NoteLoc, NoteID) << (*PI)->getString()
+ << (*PI)->getRanges();
+ }
+
if (!IncludePath)
continue;
+ // Then, add the path notes if necessary.
PathPieces FlatPath = PD->path.flatten(/*ShouldFlattenMacros=*/true);
for (PathPieces::const_iterator PI = FlatPath.begin(),
- PE = FlatPath.end();
- PI != PE; ++PI) {
+ PE = FlatPath.end(); PI != PE; ++PI) {
+if (isa(PI->get()))
+ continue;
+
SourceLocation NoteLoc = (*PI)->getLocation().asLocation();
Diag.Report(NoteLoc, NoteID) << (*PI)->getString()
<< (*PI)->getRanges();
Index: lib/StaticAnalyzer/Core/PlistDiagnostics.cpp
===
--- lib/StaticAnalyzer/Core/PlistDiagnostics.cpp
+++ lib/StaticAnalyzer/Core/PlistDiagnostics.cpp
@@ -281,6 +281,9 @@
ReportMacro(o, cast(P), FM, SM, LangOpts,
indent, depth);
break;
+case PathDiagnosticPiece::ExtraNote:
+ // FIXME: Extend the plist format to support those.
+ break;
}
}
Index: lib/StaticAnalyzer/Core/PathDiagnostic.cpp
===
--- lib/StaticAnalyzer/Core/PathDiagnostic.cpp
+++ lib/StaticAnalyzer/Core/PathDiagnostic.cpp
@@ -60,6 +60,7 @@
PathDiagnosticCallPiece::~PathDiagnosticCallPiece() {}
PathDiagnosticControlFlowPiece::~PathDiagnosticControlFlowPiece() {}
PathDiagnosticMacroPiece::~PathDiagnosticMacroPiece() {}
+PathDiagnosticExtraNotePiece::~PathDiagnosticExtraNotePiece() {}
void PathPieces::flattenTo(PathPieces , PathPieces ,
bool ShouldFlattenMacros) const {
@@ -95,6 +96,7 @@
}
case PathDiagnosticPiece::Event:
case PathDiagnosticPiece::ControlFlow:
+case PathDiagnosticPiece::ExtraNote:
Current.push_back(Piece);
break;
}
@@ -342,15 +344,16 @@
}
switch (X.getKind()) {
-case clang::ento::PathDiagnosticPiece::ControlFlow:
+case PathDiagnosticPiece::ControlFlow:
return compareControlFlow(cast(X),
cast(Y));
-case clang::ento::PathDiagnosticPiece::Event:
+case PathDiagnosticPiece::Event:
+case PathDiagnosticPiece::ExtraNote:
return None;
-case clang::ento::PathDiagnosticPiece::Macro:
+case PathDiagnosticPiece::Macro:
return compareMacro(cast(X),
cast(Y));
-case clang::ento::PathDiagnosticPiece::Call:
+case PathDiagnosticPiece::Call:
return compareCall(cast(X),
cast(Y));
}
@@ -1098,6 +1101,10 @@
ID.Add(**I);
}
+void PathDiagnosticExtraNotePiece::Profile(llvm::FoldingSetNodeID ) const {
+ PathDiagnosticSpotPiece::Profile(ID);
+}
+
void PathDiagnostic::Profile(llvm::FoldingSetNodeID ) const {
ID.Add(getLocation());
ID.AddString(BugType);
Index: lib/StaticAnalyzer/Core/HTMLDiagnostics.cpp
===
--- lib/StaticAnalyzer/Core/HTMLDiagnostics.cpp
+++ lib/StaticAnalyzer/Core/HTMLDiagnostics.cpp
@@ -152,13 +152,29 @@
}
// Process the path.
- unsigned
Re: [PATCH] D24278: [analyzer] Extend bug reports with extra notes.
dcoughlin added a comment. This is awesome! I have some minor comments on the dealloc notes. It is also fine to remove them entirely; we can add them in a later commit. Comment at: lib/StaticAnalyzer/Checkers/CheckObjCDealloc.cpp:604 @@ -588,1 +603,3 @@ +addExtraNoteForDecl(BR, "The property is declared here", PropDecl); + To be consistent with the rest of clang, I think it is better to remove the "The". That is: "Property is declared here" and "Property is synthesized here". Comment at: lib/StaticAnalyzer/Checkers/CheckObjCDealloc.cpp:733 @@ -712,1 +732,3 @@ + addExtraNoteForDecl(BR, "The property is declared here", PropDecl); + Is it possible to factor this out so we only have the hard-coded note text for each of "declared"/"synthesized" in one spot? This will make it easier to update the text if we decide to change it. https://reviews.llvm.org/D24278 ___ cfe-commits mailing list cfe-commits@lists.llvm.org http://lists.llvm.org/cgi-bin/mailman/listinfo/cfe-commits
Re: [PATCH] D24278: [analyzer] Extend bug reports with extra notes.
zaks.anna added a comment.
Thanks!
Looks good overall. Several comments below.
Comment at: lib/StaticAnalyzer/Core/HTMLDiagnostics.cpp:160
@@ +159,3 @@
+[](const IntrusiveRefCntPtr ) {
+ return isa(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(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
Re: [PATCH] D24278: [analyzer] Extend bug reports with extra notes.
bcraig added a subscriber: bcraig. bcraig added a comment. Neat! I would have liked to have had this for the Excess Padding Checker. Currently, the padding checker has a very long diagnostic that recommends a new order for data members. I think a note (or fixit) would be more appropriate, but that support didn't exist previously. https://reviews.llvm.org/D24278 ___ cfe-commits mailing list cfe-commits@lists.llvm.org http://lists.llvm.org/cgi-bin/mailman/listinfo/cfe-commits
Re: [PATCH] D24278: [analyzer] Extend bug reports with extra notes.
NoQ added a comment.
Thanks!
I could have split this up into three patches (one for the core and two patches
for the checkers), however that'd mean that the first patch comes without
tests; so i thought that the patch should be self-contained. Was it a bad idea
after all?
Comment at: lib/StaticAnalyzer/Core/BugReporter.cpp:3449
@@ +3448,3 @@
+// we may optionally convert those to path notes.
+for (auto I = exampleReport->getExtraNotes().rbegin(),
+ E = exampleReport->getExtraNotes().rend(); I != E; ++I) {
a.sidorin wrote:
> Reverse iteration on array and push the corresponding element to the front
> (always O(n)) seems a bit strange to me. I suggest using a combination of
> resize + move existing elements to the end + copy/transform from start. What
> do you think? Or am I missing something?
`PathPieces` is an `std::list`, so each push is O(1).
https://reviews.llvm.org/D24278
___
cfe-commits mailing list
cfe-commits@lists.llvm.org
http://lists.llvm.org/cgi-bin/mailman/listinfo/cfe-commits
Re: [PATCH] D24278: [analyzer] Extend bug reports with extra notes.
a.sidorin added a comment.
This definitely seems to be useful. However, this patch is pretty big. Some of
its parts are not directly related with the feature being introduced (for
example, changes for copypaste/sub-sequences.cpp). Is it possible to split this
patch? Moreover, as I understand, you may not even need a review for
refactoring-only changes. Or you can make a review for them which will be done
much faster.
There is a temporary dump of some stylish comments after brief look.
Comment at: lib/StaticAnalyzer/Core/BugReporter.cpp:3449
@@ +3448,3 @@
+// we may optionally convert those to path notes.
+for (auto I = exampleReport->getExtraNotes().rbegin(),
+ E = exampleReport->getExtraNotes().rend(); I != E; ++I) {
Reverse iteration on array and push the corresponding element to the front
(always O(n)) seems a bit strange to me. I suggest using a combination of
resize + move existing elements to the end + copy/transform from start. What do
you think? Or am I missing something?
Comment at: lib/StaticAnalyzer/Core/HTMLDiagnostics.cpp:230
@@ +229,3 @@
+unsigned NumExtraPieces = 0;
+for (auto I = path.begin(), E = path.end(); I != E; ++I) {
+ if (const auto *P = dyn_cast(I->get())) {
const auto : path?
Comment at: lib/StaticAnalyzer/Frontend/AnalysisConsumer.cpp:117
@@ +116,3 @@
+ // First, add extra notes, even if paths should not be included.
+ for (PathPieces::const_iterator PI = PD->path.begin(),
+ PE = PD->path.end(); PI != PE; ++PI) {
Range-for loop may look nicer here. What do you think?
Comment at: lib/StaticAnalyzer/Frontend/AnalysisConsumer.cpp:119
@@ +118,3 @@
+ PE = PD->path.end(); PI != PE; ++PI) {
+if (!isa(PI->get()))
+ continue;
if (isa<...>()) {
...
...
}
?
https://reviews.llvm.org/D24278
___
cfe-commits mailing list
cfe-commits@lists.llvm.org
http://lists.llvm.org/cgi-bin/mailman/listinfo/cfe-commits
Re: [PATCH] D24278: [analyzer] Extend bug reports with extra notes.
v.g.vassilev added a comment.
Thanks for working on this!
On my browser the note "Detected code clone" note appears slightly off the
highlighted range which was a bit confusing to me.
Given my limited understanding of the SA bug reports, this looks good to me.
Comment at: lib/StaticAnalyzer/Checkers/CloneChecker.cpp:83
@@ +82,3 @@
+static PathDiagnosticLocation makeLocation(const StmtSequence ,
+ AnalysisManager ) {
+ ASTContext = Mgr.getASTContext();
Probably an indent here would make this look more consistent.
Comment at: lib/StaticAnalyzer/Core/AnalyzerOptions.cpp:348
@@ +347,3 @@
+
+
+bool AnalyzerOptions::shouldDisplayExtraNotesAsEvents() {
Extra new line?
https://reviews.llvm.org/D24278
___
cfe-commits mailing list
cfe-commits@lists.llvm.org
http://lists.llvm.org/cgi-bin/mailman/listinfo/cfe-commits
