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
_______________________________________________
cfe-commits mailing list
[email protected]
http://lists.cs.uiuc.edu/mailman/listinfo/cfe-commits
