steakhal requested changes to this revision.
steakhal added a comment.
This revision now requires changes to proceed.
Looks great. It'll be a nice addition.
It feels like `SymbolCast` is a `UnarySymExpr`, but I like to have a distinct
entity for this.
Anyway. There are a lot more places where we visit and handle all SymExpr kinds.
Such as:
`SValExplainer`, `SymbolExpressor`, `SymbolicRangeInferrer`,
`SimpleSValBuilder::simplifySValOnce()::Simplifier`, `TypeRetrievingVisitor`.
Out of these, I think `SValExplainer`, `SymbolExpressor`, and
`TypeRetrievingVisitor` are the crucial ones to have consistent behavior.
On top of these, the rest of them e.g. the constraint manager and
simplification just improve the reasoning about these, so they probably can be
done incrementally.
================
Comment at:
clang/include/clang/StaticAnalyzer/Core/PathSensitive/SymbolManager.h:321
+ : SymExpr(UnarySymExprKind), Operand(In), Op(Op), T(T) {
+ assert(classof(this));
+ // Unary expressions are results of arithmetic. Pointer arithmetic is not
----------------
This is always true, right?
================
Comment at:
clang/include/clang/StaticAnalyzer/Core/PathSensitive/SymbolManager.h:325
+ // sub-regions to regions.
+ assert(isValidTypeForSymbol(T) && !Loc::isLocType(T));
+ }
----------------
I would prefer distinct assertions. This way if it triggers we can know which
condition is violated.
================
Comment at:
clang/include/clang/StaticAnalyzer/Core/PathSensitive/SymbolManager.h:352
+
+ // Implement isa<T> support.
+ static bool classof(const SymExpr *SE) {
----------------
I see that all the rest of the classes have this comment, but I found them not
too useful.
================
Comment at: clang/lib/StaticAnalyzer/Core/SValBuilder.cpp:104
+ QualType type) {
+ assert(operand);
+ assert(!Loc::isLocType(type));
----------------
I think we should assert that we expect only `UO_Minus` (-), `UO_Not` (~).
The rest of the `UO_*` doesn't seem to be relevant anyway: PostInc, PostDec,
PreInc, PreDec, AddrOf, Deref, Plus, LNot, Real, Imag, Extension, Coawait.
================
Comment at: clang/lib/StaticAnalyzer/Core/SimpleSValBuilder.cpp:105-106
return X.castAs<nonloc::ConcreteInt>().evalComplement(*this);
default:
return UnknownVal();
}
----------------
Also handle it here.
================
Comment at: clang/lib/StaticAnalyzer/Core/SymbolManager.cpp:73-76
+void UnarySymExpr::dumpToStream(raw_ostream &os) const {
+ os << UnaryOperator::getOpcodeStr(Op);
+ Operand->dumpToStream(os);
+}
----------------
Have a test for this.
Repository:
rG LLVM Github Monorepo
CHANGES SINCE LAST ACTION
https://reviews.llvm.org/D125318/new/
https://reviews.llvm.org/D125318
_______________________________________________
cfe-commits mailing list
cfe-commits@lists.llvm.org
https://lists.llvm.org/cgi-bin/mailman/listinfo/cfe-commits
