[PATCH] D41538: [analyzer] Fix some checker's output plist not containing the checker name #2

[Aleksei Sidorin via Phabricator via cfe-commits]

a.sidorin added a comment.

Hello Gabor,

For me, this patch looks much better than previous. I have some questions inline.



================
Comment at: include/clang/StaticAnalyzer/Core/BugReporter/BugType.h:40
 public:
-  BugType(class CheckName check, StringRef name, StringRef cat)
-      : Check(check), Name(name), Category(cat), SuppressonSink(false) {}
-  BugType(const CheckerBase *checker, StringRef name, StringRef cat)
-      : Check(checker->getCheckName()), Name(name), Category(cat),
-        SuppressonSink(false) {}
-  virtual ~BugType() {}
-
-  // FIXME: Should these be made strings as well?
-  StringRef getName() const { return Name; }
+  BugType(class CheckName Check, StringRef Name, StringRef Cat)
+      : Check(Check), Name(Name), Category(Cat), Checker(nullptr),
----------------
Hm, do we really want to use elaborated name here: `class CheckName`?


================
Comment at: include/clang/StaticAnalyzer/Core/BugReporter/BugType.h:50
+    // FIXME: This is a workaround to ensure that Check is initialized 
+    // correctly. The name of the checks are set after checker the constructor
+    // is run. In case the BugType object is initialized in the checker's ctor
----------------
Looks like a misspell. Should  it be "The check names are set after the constructors are run"?


================
Comment at: include/clang/StaticAnalyzer/Core/BugReporter/BugType.h:54
+    // initialization.
+    if (getCheckName().empty() && Checker) {
+      Check = Checker->getCheckName();
----------------
Possibly I am missing the context, but could you please explain, why do we modify CheckName in getName() but not in getCheckName()? It seems a bit unclear to me.


================
Comment at: lib/StaticAnalyzer/Checkers/ValistChecker.cpp:277
+      BT_leakedvalist.reset(new BugType(
+          CheckNames[ReportUninit ? CK_Uninitialized : CK_Unterminated],
+          "Leaked va_list", categories::MemoryError));
----------------
If I understand correctly, if we report uninitialized and then unterminated, the second report will have wrong CheckName because it is never reset.


================
Comment at: test/Analysis/malloc.c:1723
 
-char *dupstrWarn(const char *s) {
-  const int len = strlen(s);
----------------
Should we enable the checker instead of removing test?


Repository:
  rC Clang

https://reviews.llvm.org/D41538