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<PathDiagnosticExtraNotePiece>(I->get())) {
----------------
const auto &Piece : 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<PathDiagnosticExtraNotePiece>(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
