martong added inline comments.
================
Comment at: clang/include/clang/StaticAnalyzer/Core/Analyses.def:17
-ANALYSIS_STORE(RegionStore, "region", "Use region-based analyzer store",
CreateRegionStoreManager)
+ANALYSIS_STORE(RegionStore, "region", "Use region-based analyzer store",
+ CreateRegionStoreManager)
----------------
Yay! I am a big fun of the 80 col limit! :)
================
Comment at: clang/include/clang/StaticAnalyzer/Core/Analyses.def:61
+
+ANALYSIS_DIAGNOSTICS(TEXT_MINIMAL, "text-minimal",
+ "Emits minimal diagnostics to stderr, stating only the "
----------------
Just out of curiosity: is there a way to know which one is the default?
================
Comment at: clang/lib/StaticAnalyzer/Core/HTMLDiagnostics.cpp:140
+
+ // FIXME: HTML is currently our default output type, but if the output
+ // directory isn't specified, it acts like if it was in the minimal text
----------------
Actually, now I wonder where should we document the default output type ... I
am pretty sure it should be done somewhere where it is obvious, maybe in
`Analyses.def`?
================
Comment at: clang/lib/StaticAnalyzer/Core/HTMLDiagnostics.cpp:142
+ // directory isn't specified, it acts like if it was in the minimal text
+ // output mode. This doesn't make much sense, we should have the minimal text
+ // as our default. In the case of backward compatibility concerns, this could
----------------
Man, I understand you pain now, this is really muddled, do you plan to sort
this out in an upcoming patch?
================
Comment at: clang/lib/StaticAnalyzer/Core/HTMLDiagnostics.cpp:148
+ // TODO: Emit an error here.
+ if (OutputDir.empty())
+ return;
----------------
Would be an `assert` better here?
================
Comment at: clang/lib/StaticAnalyzer/Core/HTMLDiagnostics.cpp:148
+ // TODO: Emit an error here.
+ if (OutputDir.empty())
+ return;
----------------
martong wrote:
> Would be an `assert` better here?
```
// if the output
// directory isn't specified, it acts like if it was in the minimal text
// output mode.
```
So, why don't we do this check **before** calling
createTextMinimalPathDiagnosticConsumer ?
================
Comment at: clang/lib/StaticAnalyzer/Core/TextDiagnostics.cpp:1
+//===--- TextDiagnostics.cpp - Text Diagnostics for Paths -------*- C++
-*-===//
+//
----------------
Is this just a blind copy (and rename) from `AnalysisConsumer`, or should I
take a deeper look into anything here?
================
Comment at: clang/lib/StaticAnalyzer/Frontend/AnalysisConsumer.cpp:156
#include "clang/StaticAnalyzer/Core/Analyses.def"
- }
- }
+ default:
+ llvm_unreachable("Unkown analyzer output type!");
----------------
Some build bots will not compile if you handle all cases in the swtich and you
still have a `default`, beware.
================
Comment at: clang/lib/StaticAnalyzer/Frontend/AnalysisConsumer.cpp:348
void reportAnalyzerProgress(StringRef S);
-};
+}; // namespace
} // end anonymous namespace
----------------
Either the `;` is redundant or this is not the end of a namespace.
CHANGES SINCE LAST ACTION
https://reviews.llvm.org/D76509/new/
https://reviews.llvm.org/D76509
_______________________________________________
cfe-commits mailing list
cfe-commits@lists.llvm.org
https://lists.llvm.org/cgi-bin/mailman/listinfo/cfe-commits
