[PATCH] D153612: [clang][analyzer] Add and change NoteTags in StdLibraryFunctionsChecker.

[Donát Nagy via Phabricator via cfe-commits]

donat.nagy added a comment.

The source code changes LGTM. I'll soon check the results on the open source projects and give a final approval based on that.

By the way, I think this commit and the "display notes only if interesting" follow-up change (D153776 <https://reviews.llvm.org/D153776>) should be merged at the same time (but I'd guess that you already planned to do it that way).



================
Comment at: clang/lib/StaticAnalyzer/Checkers/StdLibraryFunctionsChecker.cpp:1299
+    // StdLibraryFunctionsChecker.
+    ExplodedNode *Pred = const_cast<ExplodedNode *>(Node);
+    if (!Case.getNote().empty()) {
----------------
balazske wrote:
> donat.nagy wrote:
> > balazske wrote:
> > > donat.nagy wrote:
> > > > Can you explain why is it safe to use `const_cast` here? (I don't see any concrete issue, but the engine has lots of invariants / unwritten rules and I fear that this might break one of them.)
> > > The node `Pred` should be modified only later when a successor is added (`addTransition` has non-const parameter).
> > I understood that you //need// a non-const `ExplodedNode *` because `addTransition` expects it; I want to understand why you are //allowed to// `const_cast` it (why doesn't this confuse the engine logic).
> > 
> > Equivalent question from the other direction: Why did the author of `CheckerContext::getPredecessor()` specify that its return value is a //const// pointer to `ExplodedNode`?
> > 
> > If we can conclude that `const_cast` is valid in this kind of situation, then I'd also consider simply removing the "const" from the return type of `getPredecessor`.
> The `const_cast` is not needed at all if `Pred` and `Node` is made non-const, and `getPredecessor` has a non-const version. The `Node` is saved because we want to add transitions to it, it makes no sense to have it (a pointer to) const. (Probably the const comes from a time when the `Node` was used only for the lambda? In the lambda it could be const, if it matters.)
Sorry, it seems that I badly misread the source code of CheckerContext.h (I probably looked at the wrong line when I briefly jumped to the definition of `getPredecessor`). In fact, `getPredecessor` has only one version and it returns non-const `ExplodedNode *`.

This means that your code is (of course) completely valid.


Repository:
  rG LLVM Github Monorepo

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

https://reviews.llvm.org/D153612