[PATCH] Expose the name of the checker producing each diagnostic message.

Alexander Kornienko alexfh at google.com
Tue Feb 11 09:48:23 PST 2014


  Jordan, Ted, thanks for the review!

  Is it now fine to commit? ;)


================
Comment at: include/clang/StaticAnalyzer/Core/BugReporter/BugType.h:40-41
@@ -37,3 +39,4 @@
 public:
-  BugType(StringRef name, StringRef cat)
-    : Name(name), Category(cat), SuppressonSink(false) {}
+  BugType(class CheckName CheckName, StringRef name, StringRef cat)
+      : CheckName(CheckName), Name(name), Category(cat),
+        SuppressonSink(false) {}
----------------
Jordan Rose wrote:
> We try not to shadow class names with field or parameter names. Just "const CheckName Check", maybe?
Done.

================
Comment at: lib/StaticAnalyzer/Checkers/MallocChecker.cpp:309-312
@@ -306,3 +308,6 @@
   /// Tells if a given family/call/symbol is tracked by the current checker.
-  bool isTrackedByCurrentChecker(AllocationFamily Family) const;
+  /// Sets CheckKind to the kind of the checker responsible for this
+  /// family/call/symbol.
+  bool isTrackedByCurrentChecker(AllocationFamily Family,
+                                 CheckKind &CheckKind) const;
   bool isTrackedByCurrentChecker(CheckerContext &C,
----------------
Jordan Rose wrote:
> Possible alternative:
> 
> ```Optional<CheckKind> getCheckIfTracked(AllocationFamily Family) const;```
> 
> I personally tend to avoid returning values by reference. What do you think of this version?
With Optional the user code becomes somewhat less elegant, but if you like it more, I'm fine with it.

================
Comment at: lib/StaticAnalyzer/Checkers/UnixAPIChecker.cpp:68-71
@@ -62,12 +67,6 @@
 
 //===----------------------------------------------------------------------===//
 // Utility functions.
 //===----------------------------------------------------------------------===//
 
 
----------------
Jordan Rose wrote:
> There are no more utility functions, so let's drop this header.
Sure. Done.

================
Comment at: lib/StaticAnalyzer/Checkers/MallocChecker.cpp:2276-2280
@@ -2235,3 +2275,7 @@
   // checker.
-  mgr.registerChecker<MallocChecker>()->Filter.CNewDeleteChecker = true;
+  if (!checker->ChecksEnabled[MallocChecker::CK_NewDeleteChecker]) {
+    checker->ChecksEnabled[MallocChecker::CK_NewDeleteChecker] = true;
+    checker->CheckNames[MallocChecker::CK_NewDeleteChecker] =
+        mgr.getCurrentCheckName();
+  }
 }
----------------
Jordan Rose wrote:
> Good work here: if only the leak-checker is enabled, it becomes responsible for the use-after-delete checks, but if both are enabled the normal NewDelete checker will show up. Nice!
MallocChecker is particularly convoluted. It should be possible to have the same functionality with a simpler structure, then this code would not be necessary =\


http://llvm-reviews.chandlerc.com/D2557



More information about the cfe-commits mailing list