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

Jordan Rose jordan_rose at apple.com
Mon Feb 10 18:32:52 PST 2014


  Ted and I talked offline about this and other changes and agreed that this is a good move. We have some notions about future work on the analyzer that I'll try to write up and send to you and cfe-dev tomorrow, but they don't seem like goals that would conflict with clang-tidy's.

  I've done a pass over the entire patch this time and come up with a few comments, but this mostly looks good. Thank you for doing all the boring work of getting this up and running in all the existing checkers!


================
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) {}
----------------
We try not to shadow class names with field or parameter names. Just "const CheckName Check", maybe?

================
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,
----------------
Possible alternative:

```Optional<CheckKind> getCheckIfTracked(AllocationFamily Family) const;```

I personally tend to avoid returning values by reference. What do you think of this version?

================
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();
+  }
 }
----------------
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!

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


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



More information about the cfe-commits mailing list