[PATCH] D10305: [Clang Static Analyzer] Bug identification

[Gábor Horváth via cfe-commits]

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