xazax.hun marked 9 inline comments as done.
================
Comment at: lib/StaticAnalyzer/Core/BugId.cpp:29
@@ +28,3 @@
+
+static std::string GetSignature(const FunctionDecl *Target) {
+ if (!Target)
----------------
zaks.anna wrote:
> Can/Should we use some existing machinery for this? For example, mangleName().
Generating mangled names requires ASTContext which is not available during the
error reporting. BugReporter does have the ASTContext, so it would not be a big
change to add it to the DiagnosticConsumers though. And I think the mangled
name might contain too much redundant information. The only relevant
information here is the fully qualified name and the parts of the signature
that can be ocerloaded on e.g.: constness. Using this method might also be
easier to debug, since the IssueString is more readable.
================
Comment at: lib/StaticAnalyzer/Core/BugId.cpp:38
@@ +37,3 @@
+
+ switch (Target->getStorageClass()) {
+ case SC_Extern:
----------------
zaks.anna wrote:
> Why are these necessary?
You are right, it is not possible to overload on these properties, so it is
safe (and even beneficial) to remove them from the hash.
================
Comment at: lib/StaticAnalyzer/Core/BugId.cpp:52
@@ +51,3 @@
+
+ if (Target->isInlineSpecified())
+ Signature.append("inline ");
----------------
zaks.anna wrote:
> Why do we need these in the hash?
We do not need these information (see my previous comment). I removed them from
the hash.
================
Comment at: lib/StaticAnalyzer/Core/BugId.cpp:198
@@ +197,3 @@
+std::string clang::GetIssueString(const SourceManager &SM, FullSourceLoc &L,
+ StringRef CheckerName, StringRef HashField,
+ const Decl *D) {
----------------
zaks.anna wrote:
> Including the checker name here is not perfect because checker name can
> easily change from one release of clang to another. For example, when a
> checker moves to another package.
>
> I think the best approach here would be to give checkers some unique
> identifiers and using those here. We can discuss the more if you are
> interested in solving this problem.
I definitely agree. It would be great to have a unique identifier for checkers
that is independent from the name/package. It is not a trivial problem however,
since we probably want to support plugins too. I can think of a similar
solution like git commit ids, but I think this problem might be out of the
scope of this patch. But I am happy to start a discussion on the mailing list
and create a new patch to solve this.
================
Comment at: lib/StaticAnalyzer/Core/PlistDiagnostics.cpp:20
@@ -19,2 +19,3 @@
#include "clang/Lex/Preprocessor.h"
+#include "clang/StaticAnalyzer/Core/AnalyzerOptions.h"
#include "clang/StaticAnalyzer/Core/BugReporter/PathDiagnostic.h"
----------------
zaks.anna wrote:
> Is this needed?
Removed.
================
Comment at: test/Analysis/model-file.cpp:43
@@ -42,7 +42,3 @@
// CHECK-NEXT: <array>
-// CHECK-NEXT: <dict>
-// CHECK-NEXT: <key>path</key>
-// CHECK-NEXT: <array>
-// CHECK-NEXT: <dict>
-// CHECK-NEXT: <key>kind</key><string>control</string>
-// CHECK-NEXT: <key>edges</key>
+// CHECK-NEXT: <dict>
+// CHECK-NEXT: <key>path</key>
----------------
zaks.anna wrote:
> Extra space in the whole file.
Whatever I do it looks like the whitespaces are not alligned well for this
file. My guess s that this plist might be generated with an old version of the
static analyzer. I think it is better to actually update it to the old
formatting than doing a diff by hand, or writing a script to do just that.
http://reviews.llvm.org/D10305
_______________________________________________
cfe-commits mailing list
cfe-commits@lists.llvm.org
http://lists.llvm.org/cgi-bin/mailman/listinfo/cfe-commits
