[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.

This isn't an NFC change. It's more than a refactor/cosmetic change.
Please run some benchmarks to validate the assumed performance characteristics.



================
Comment at: clang/include/clang/StaticAnalyzer/Core/PathSensitive/ConstraintManager.h:156
+  private:
+    llvm::SmallSet<const ProgramState *, 4> Aux;
+  };
----------------
Have you considered [[ https://www.llvm.org/docs/ProgrammersManual.html#llvm-adt-smallptrset-h | llvm::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);
----------------
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.


Repository:
  rG LLVM Github Monorepo

CHANGES SINCE LAST ACTION
  https://reviews.llvm.org/D126560/new/

https://reviews.llvm.org/D126560