Szelethus added subscribers: steakhal, boga95.
Szelethus added inline comments.


================
Comment at: clang/lib/StaticAnalyzer/Checkers/ContainerModeling.cpp:731
+  }
+  return C.getNoteTag([Text, Name](BugReport &BR) -> std::string {
+      SmallString<256> Msg;
----------------
NoQ wrote:
> baloghadamsoftware wrote:
> > NoQ wrote:
> > > You'll need to check whether the container is actually of interest to the 
> > > bug report. We don't want notes to be added about changes to irrelevant 
> > > containers.
> > > 
> > > You can use a combination of "Report `BR` was emitted by one of the 
> > > iterator checkers" and "The memory region of the container is marked as 
> > > interesting" (while also actually marking it as interesting in the 
> > > checker).
> > > 
> > > Ideally we should instead make a new generic storage inside the 
> > > `BugReport` object, in order to pass down the interesting information 
> > > from the call site of `emitReport` ("Hi, i'm an iterator checker who 
> > > emitted this report and i'm interested in changes made to the size of 
> > > this container").
> > Are you sure in this? I already wondered how it works so I added a test 
> > that checks one container and changes another one and there were no note 
> > tags displayed for the one we did not check but change. See the last test.
> That's because you didn't do
> ```lang=c++
>   V2.cbegin();
>   V2.cend();
> ```
> in the beginning.
A similar conversation sparked up recently in between @boga95, @steakhal and me 
regarding reporting taintedness. Bug reports are fine up to the point where (in 
reverse) the first propagation happens, but finding out which value tainted the 
one that caused the report isn't handled at the moment. One idea was to mark 
the initial (again, in reverse) value as interesting, create a `NoteTag` at the 
point of propagation, where we should know which value was the cause of the 
spread, mark that interesting as well, etc.

If `NoteTag`s only emit a message when the concerning value is interesting, 
this should theoretically solve that problem. I guess you could say that we're 
propagating interestingness in reverse.

I'm not immediately sure if this idea was ever mentioned or implemented here.


================
Comment at: clang/lib/StaticAnalyzer/Checkers/ContainerModeling.cpp:58
 public:
   ContainerModeling() {}
 
----------------
While we're at it [part 2], can we make this `= default`? :)


================
Comment at: clang/lib/StaticAnalyzer/Checkers/ContainerModeling.cpp:68-69
+                                                  SVal) const;
+  typedef void (ContainerModeling::*TwoItParamFn)(CheckerContext &, SVal, SVal,
+                                                  SVal) const;
 
----------------
Prefer `using`.


CHANGES SINCE LAST ACTION
  https://reviews.llvm.org/D73720/new/

https://reviews.llvm.org/D73720



_______________________________________________
cfe-commits mailing list
cfe-commits@lists.llvm.org
https://lists.llvm.org/cgi-bin/mailman/listinfo/cfe-commits

Reply via email to