Szelethus added a comment.
I know I'm late to the party, and am just mostly thinking aloud. Awesome patch
series!
================
Comment at:
clang/include/clang/StaticAnalyzer/Core/BugReporter/BugReporterVisitors.h:331-341
+/// Visitor that tracks expressions and values.
+class TrackingBugReporterVisitor : public BugReporterVisitor {
+private:
+ TrackerRef ParentTracker;
+
+public:
+ TrackingBugReporterVisitor(TrackerRef ParentTracker)
----------------
Maybe we could draw more attention to the fact this is a major component of bug
report construction? Dividers like this maybe:
```
//===----------------------------------------------------------------------===//
// Visitor core components.
//===----------------------------------------------------------------------===//
class BugReporterVisitor {};
// ...
class TrackingBugReporterVisitor {};
//===----------------------------------------------------------------------===//
// Specific visitors.
//===----------------------------------------------------------------------===//
```
================
Comment at:
clang/include/clang/StaticAnalyzer/Core/BugReporter/BugReporterVisitors.h:340
+
+ Tracker &getParentTracker() { return *ParentTracker; }
+};
----------------
Why is this a //parent// tracker? Could visitors have their own child trackers?
================
Comment at:
clang/include/clang/StaticAnalyzer/Core/BugReporter/BugReporterVisitors.h:367-368
+/// \param Origin Only adds notes when the last store happened in a
+/// different stackframe to this one. Disregarded if the tracking kind
+/// is thorough.
+/// This is useful, because for non-tracked regions, notes about
----------------
vsavchenko wrote:
> Szelethus wrote:
> > Different or nested? (D64272)
> I'm not sure I understand this question, but it is simply a refactoring, all
> parameters come from previous interfaces.
Oh well, then its likely me that made this mistake :^)
Both `f`'s and `h`'s stack frame is different to `g`'s, but only `f` is nested.
As a top level function, the bug path would start from `h`, so arrows and
function call events would be present in it. `f` may be pruned (no diagnostic
pieces would point to it), if the analyzer decides that nothing interesting
happens in it (not this, but maybe a similar example). Following this logic,
we're looking for nested stack frames in particular, to tell the analyzer not
to prune them.
```lang=c++
void f(int **p) {
*p = NULL; // At the point of assignment, the stackframe of f is nested in
g's stackframe
}
void h() {
*p = malloc(...);
g(p); // h calls g, g's stackframe is nested in h's
}
void g(int **p) {
f(p);
**p = 5; // warning: nullptr reference
}
```
The comment is a little dumb, as it explicitly mentions "nested" a sentence
later.
Repository:
rG LLVM Github Monorepo
CHANGES SINCE LAST ACTION
https://reviews.llvm.org/D103618/new/
https://reviews.llvm.org/D103618
_______________________________________________
cfe-commits mailing list
cfe-commits@lists.llvm.org
https://lists.llvm.org/cgi-bin/mailman/listinfo/cfe-commits
