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

[Artem Dergachev via Phabricator via cfe-commits]

NoQ accepted this revision.
NoQ added inline comments.
This revision is now accepted and ready to land.


================
Comment at: include/clang/StaticAnalyzer/Core/BugReporter/BugType.h:51
   StringRef getCategory() const { return Category; }
-  StringRef getCheckName() const { return Check.getName(); }
+  StringRef getCheckName() const {
+    // FIXME: This is a workaround to ensure that Check is initialized 
----------------
xazax.hun wrote:
> NoQ wrote:
> > I suggest:
> > 
> > ```
> > if (Checker)
> >   return Checker->getCheckname().getName();
> > return Check.getName();
> > ```
> > 
> > Like, what's the point of storing the StringRef if we can retrieve it any time anyway?
> > 
> > I guess `Checker` does live long enough?
> I slightly modified your snippet to preserve the assertion that can be useful to not to reintroduce this bug in the future.
Yep, totally!


================
Comment at: lib/StaticAnalyzer/Checkers/ValistChecker.cpp:277
+      BT_leakedvalist.reset(new BugType(
+          CheckNames[ReportUninit ? CK_Uninitialized : CK_Unterminated],
+          "Leaked va_list", categories::MemoryError));
----------------
xazax.hun wrote:
> NoQ wrote:
> > a.sidorin wrote:
> > > xazax.hun wrote:
> > > > 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.
> > > Maybe we should use different BugTypes for Uninitialized and Unterminated?
> > Yep, this rings my bells too. We shouldn't race on how do we initialize a `BugType` depending on what bug is encountered first in the translation unit.
> Maybe I am missing something but right now there is no race on how we initialize the `BugType`. The result of the initialization can depend on, however, the set of checks that are registered. I agree that this is unfortunate but introducing a new `BugType` does not solve this problem or at least it is not apparent for me how would it solve this. One option would be to simply not report these issues unless the corresponding check is registered. The other option would be to make this depend on a new check name that needs to be enabled.
Aha, that's right, yep, so this BugType's checker name would be different depending on which checks are enabled, not on bug first encountered, sorry. So it'd only have any effect if someone is grepping through bug types and turns these checkers on and off individually at the same time. Still worth a FIXME i guess? Is it hard to make a third bugtype with a hardcoded name that doesn't rely on the checker?


https://reviews.llvm.org/D41538