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

Aleksei Sidorin via Phabricator via cfe-commits cfe-commits at lists.llvm.org
Wed Dec 27 10:59:12 PST 2017

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
-  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?

  rC Clang


More information about the cfe-commits mailing list