steakhal added a comment. In D126560#3549570 <https://reviews.llvm.org/D126560#3549570>, @martong wrote:
> In D126560#3549408 <https://reviews.llvm.org/D126560#3549408>, @steakhal > wrote: > >> This isn't an NFC change. It's more than a refactor/cosmetic change. > > Still, there is no visible change in the behavior, at least that is intended. > Should it be an NFCi ? > >> Please run some benchmarks to validate the assumed performance >> characteristics. > > Yeah I've thought of it, here it is: > F23275670: image.png <https://reviews.llvm.org/F23275670> Yes, let's go with NFCi. ================ 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); ---------------- martong wrote: > 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. > > > I'd like to keep the interface of hasCycle and push and pop because those > express the intention more clearly. I disagree. Double lookups are always a code-smell. I insist on this. 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
