martong marked 2 inline comments as done. martong added inline comments.
================ Comment at: clang/include/clang/StaticAnalyzer/Core/PathSensitive/ConstraintManager.h:156 + private: + llvm::SmallSet<const ProgramState *, 4> Aux; + }; ---------------- steakhal wrote: > Have you considered [[ > https://www.llvm.org/docs/ProgrammersManual.html#llvm-adt-smallptrset-h | > llvm::SmallPtrSet]] ? Actually, a SmallSet of pointers is transparently implemented with a SmallPtrSet. ================ Comment at: clang/lib/StaticAnalyzer/Core/ConstraintManager.cpp:56-59 + const ProgramState *RawSt = State.get(); + if (AssumeStack.hasCycle(RawSt)) + return {State, State}; + AssumeStack.push(RawSt); ---------------- steakhal wrote: > Why don't you return `RawSt` on the early return path instead? That seems to > contain more information than `State`, right? > > You lookup twice the location of the `RawSt` entry; once in the `hasCycle()` > and again in the `push()`. > I would recommend doing an unconditional `insert()` directly and reusing the > returned bool part of the pair to observe if it did actually insert. > Why don't you return `RawSt` on the early return path instead? That seems to > contain more information than `State`, right? No, `RawSt` contains the same information as `State`. > You lookup twice the location of the `RawSt` entry; once in the `hasCycle()` > and again in the `push()`. > I would recommend doing an unconditional `insert()` directly and reusing the > returned bool part of the pair to observe if it did actually insert. I am considering to change the container to a `SmallVector` instead. I'd like to keep the interface of `hasCycle` and `push` and `pop` because those express the intention more clearly. And SmallSet falls back to a simple linear search anyway if size is less than N. Repository: rG LLVM Github Monorepo CHANGES SINCE LAST ACTION https://reviews.llvm.org/D126560/new/ https://reviews.llvm.org/D126560 _______________________________________________ cfe-commits mailing list cfe-commits@lists.llvm.org https://lists.llvm.org/cgi-bin/mailman/listinfo/cfe-commits
