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

[Anna Zaks via cfe-commits]

zaks.anna added a comment.

Could you address this:

Could you ask on the open source list if people are using "issue_hash" and if they are Ok with us renaming it.

I'd suggest to suffix each issue hash field with the description of that hash. For example, we would remove "issue_hash" and replace it with "issue_hash_function_offset" and add "issue_hash_content_of _line_in_context".


================
Comment at: include/clang/StaticAnalyzer/Core/BugId.h:1
@@ +1,2 @@
+//===---------- BugId.h - Unique bugid utility ------------------*- C++ -*-===//
+//
----------------
Please, rename BugId into issue hash everywhere, including the file names.

================
Comment at: include/clang/StaticAnalyzer/Core/BugId.h:23
@@ +22,3 @@
+
+llvm::SmallString<32> GetIssueHash(const SourceManager &SM, FullSourceLoc &L,
+                                   llvm::StringRef CheckerName,
----------------
This is an API. Could you add descriptions to the parameter and/or use better parameter names. For example, what's HashField?

We also need to describe how the hash is computed; for example, how L is used...

================
Comment at: lib/StaticAnalyzer/Checkers/DebugCheckers.cpp:239
@@ +238,3 @@
+
+    C.emitReport(llvm::make_unique<BugReport>(*BT, HashContent, N));
+  }
----------------
Awesome!

================
Comment at: lib/StaticAnalyzer/Core/BugId.cpp:12
@@ +11,3 @@
+#include "clang/AST/DeclCXX.h"
+#include "clang/Lex/Lexer.h"
+#include "clang/Basic/Specifiers.h"
----------------
Let's sort the includes.


================
Comment at: lib/StaticAnalyzer/Core/BugId.cpp:29
@@ +28,3 @@
+
+static std::string GetSignature(const FunctionDecl *Target) {
+  if (!Target)
----------------
Can/Should we use some existing machinery for this? For example, mangleName().

================
Comment at: lib/StaticAnalyzer/Core/BugId.cpp:38
@@ +37,3 @@
+
+  switch (Target->getStorageClass()) {
+  case SC_Extern:
----------------
Why are these necessary? 

================
Comment at: lib/StaticAnalyzer/Core/BugId.cpp:52
@@ +51,3 @@
+
+  if (Target->isInlineSpecified())
+    Signature.append("inline ");
----------------
Why do we need these in the hash?

================
Comment at: lib/StaticAnalyzer/Core/BugId.cpp:126
@@ +125,3 @@
+    case Decl::CXXMethod:
+    case Decl::ObjCMethod:
+    case Decl::Function:
----------------
ObjCMethod is not a FunctionDecl. Please, add a test case.

================
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) {
----------------
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.

================
Comment at: lib/StaticAnalyzer/Core/HTMLDiagnostics.cpp:242
@@ +241,3 @@
+    FullSourceLoc UL(SMgr.getExpansionLoc(UPDLoc.asLocation()), SMgr);
+    FullSourceLoc L(SMgr.getExpansionLoc(D.getLocation().asLocation()), SMgr);
+    const Decl *DeclWithIssue = D.getDeclWithIssue();
----------------
This check UPDLoc.isValid() should probably move here; no need to create 2 FullSourceLocs. It's a bit confusing to read otherwise. (Same in the code below.)

================
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"
----------------
Is this needed?

================
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>
----------------
Extra space in the whole file.


http://reviews.llvm.org/D10305