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

Tue Jun 27 08:07:38 PDT 2023

balazske added inline comments.

================
Comment at: clang/lib/StaticAnalyzer/Checkers/StdLibraryFunctionsChecker.cpp:1299
+    // StdLibraryFunctionsChecker.
+    ExplodedNode *Pred = const_cast<ExplodedNode *>(Node);
+    if (!Case.getNote().empty()) {
----------------
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.)

Repository:
  rG LLVM Github Monorepo

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

https://reviews.llvm.org/D153612