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

Gábor Horváth via Phabricator via cfe-commits cfe-commits at lists.llvm.org
Thu Jan 4 10:24:21 PST 2018


xazax.hun added inline comments.


================
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 
----------------
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.


================
Comment at: lib/StaticAnalyzer/Checkers/ValistChecker.cpp:277
+      BT_leakedvalist.reset(new BugType(
+          CheckNames[ReportUninit ? CK_Uninitialized : CK_Unterminated],
+          "Leaked va_list", categories::MemoryError));
----------------
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.


https://reviews.llvm.org/D41538





More information about the cfe-commits mailing list