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