[PATCH] D126560: [analyzer][NFC] SimpleSValBuilder simplification: Remove superfluous workaround code and track Assume call stack rather

[Balázs Benics via Phabricator via cfe-commits]

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