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

[Gabor Marton via Phabricator via cfe-commits]

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