Szelethus added inline comments.
Herald added a subscriber: manas.

================
Comment at: 
clang/lib/StaticAnalyzer/Checkers/StdLibraryFunctionsChecker.cpp:156-157
     // the bug is reported.
-    virtual std::string describe(ProgramStateRef State,
+    virtual std::string describe(DescritptionKind DK, ProgramStateRef State,
                                  const Summary &Summary) const {
       // There are some descendant classes that are not used as argument
----------------
How about we turn this into a print-like function and instead of returning with 
a string, we take an `llvm::raw_ostream` object as argument? `SmallString` + 
`raw_svector_stream` is how we construct most of our checker message strings.


================
Comment at: clang/lib/StaticAnalyzer/Checkers/StdLibraryFunctionsChecker.cpp:857
+            if (BR.isInteresting(ArgSVal))
+              OS << Msg;
+          }));
----------------
steakhal wrote:
> Ah, there is a slight issue.
> You should mark some stuff interesting here, to make this interestingness 
> propagate back transitively.
> 
> Let's say `ArgSVal` is `x + y` which is considered to be out of range 
> `[42,52]`. We should mark both `x` and `y` interesting because they 
> themselves could have been constrained by the StdLibChecker previously. So, 
> they must be interesting as well.
> 
> On the same token, IMO `PathSensitiveBugReport::markInteresting(symbol)` 
> should be //transitive//. So that all `SymbolData` in that symbolic 
> expression tree are considered interesting. What do you think @NoQ?
> If we were doing this, @martong  - you could simply acquire the assumption 
> you constructed for the given `ValueConstraint`, and mark that interesting. 
> Then all `SymbolData`s on both sides of the logic operator would become 
> implicitly interesting.
>On the same token, IMO PathSensitiveBugReport::markInteresting(symbol) should 
>be transitive. So that all SymbolData in that symbolic expression tree are 
>considered interesting. What do you think @NoQ?

Thats how I'd expect this to work. This shouldn't be a burden on the checker 
developer (certainly not this kind of a checker), but rather be handled by 
`PathSensitiveBugReport`.

So I think this is fine as it is.


Repository:
  rG LLVM Github Monorepo

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

https://reviews.llvm.org/D101526

_______________________________________________
cfe-commits mailing list
cfe-commits@lists.llvm.org
https://lists.llvm.org/cgi-bin/mailman/listinfo/cfe-commits
  • [PATCH] D101526: [analyzer... Kristóf Umann via Phabricator via cfe-commits

Reply via email to