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 _______________________________________________ cfe-commits mailing list cfe-commits@lists.llvm.org https://lists.llvm.org/cgi-bin/mailman/listinfo/cfe-commits
