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 _______________________________________________ cfe-commits mailing list cfe-commits@lists.llvm.org http://lists.llvm.org/cgi-bin/mailman/listinfo/cfe-commits