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

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

xazax.hun added inline comments.


================
Comment at: include/clang/StaticAnalyzer/Core/BugReporter/BugType.h:54
+    // initialization.
+    if (getCheckName().empty() && Checker) {
+      Check = Checker->getCheckName();
----------------
a.sidorin wrote:
> 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.
Good catch, it is just an oversight. I got confused by `CheckName` vs `Name`. And since the analyzer called `getName` before `getCheckName` the tests passed. 


================
Comment at: lib/StaticAnalyzer/Checkers/ValistChecker.cpp:277
+      BT_leakedvalist.reset(new BugType(
+          CheckNames[ReportUninit ? CK_Uninitialized : CK_Unterminated],
+          "Leaked va_list", categories::MemoryError));
----------------
a.sidorin wrote:
> If I understand correctly, if we report uninitialized and then unterminated, the second report will have wrong CheckName because it is never reset.
That is right. Unfortunately, If unterminated check is not turned on but uninitialized is, we can end up with empty check names. I replaced this workaround with a slightly more robust one.


================
Comment at: test/Analysis/malloc.c:1723
 
-char *dupstrWarn(const char *s) {
-  const int len = strlen(s);
----------------
a.sidorin wrote:
> Should we enable the checker instead of removing test?
I tried that once, but this caused more new warnings (and sinks) so more changes would be required to make the tests pass. And this case is already tested in `string.c`.


https://reviews.llvm.org/D41538