[PATCH] D77792: [analyzer] Extend constraint manager to be able to compare simple SymSymExprs

[Balogh, Ádám via Phabricator via cfe-commits]

baloghadamsoftware added inline comments.


================
Comment at: clang/lib/StaticAnalyzer/Core/RangeConstraintManager.cpp:814
 
+Optional<ProgramStateRef> RangeConstraintManager::tryAssumeSymSymOp(
+    ProgramStateRef State, BinaryOperator::Opcode Op, SymbolRef LHSSym,
----------------
NoQ wrote:
> I believe you don't need to return an optional here. The method simply acknowledges any assumptions it could make in the existing state and returns the updated state. Therefore, if it wasn't able to record any assumptions, it returns the existing state. Because the only reasonable behavior the caller could implement when the current implementation returns `None` is to roll back to the existing state anyway.
Actually, `tryAssumeSymSymOp()` does not assume anything and therefore it does not return a new state. What it actually returns is a ternary logical value: the assumption is either valid, invalid or we cannot reason about it. Maybe Optional<bool> would be better here and we should also chose a less misleading name, because it does not "try to assume" anything, but tries to reason about the existing assumption.


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

https://reviews.llvm.org/D77792