vsavchenko requested changes to this revision.
vsavchenko added a comment.
This revision now requires changes to proceed.
Hey, great work! I think that casts are extremely important, but it looks like
you mixed so many things into this patch. Let's make one step at a time a
split it into (at least) a couple of patches.
================
Comment at: clang/lib/StaticAnalyzer/Core/RangeConstraintManager.cpp:757-767
markDisequal(BasicValueFactory &BV, RangeSet::Factory &F,
- ProgramStateRef State, SymbolRef First, SymbolRef Second);
+ ProgramStateRef State, NominalTypeList &NTL, SymbolRef First,
+ SymbolRef Second);
LLVM_NODISCARD static inline ProgramStateRef
markDisequal(BasicValueFactory &BV, RangeSet::Factory &F,
- ProgramStateRef State, EquivalenceClass First,
- EquivalenceClass Second);
+ ProgramStateRef State, NominalTypeList &NTL,
+ EquivalenceClass First, EquivalenceClass Second);
----------------
That's definitely regresses the interface, so `NominalTypeList` should be
definitely reworked.
================
Comment at: clang/lib/StaticAnalyzer/Core/RangeConstraintManager.cpp:1015-1042
+class NominalTypeList {
+ CanQualType Types[4];
+
+public:
+ using Iterator = CanQualType *;
+
+ NominalTypeList(ASTContext &C)
----------------
This looks like a very `static` data structure to me, I don't see any reasons
why the user should be able to create multiple copies of it.
If it becomes a static data-structure, there will be no need in passing it
around.
================
Comment at: clang/lib/StaticAnalyzer/Core/RangeConstraintManager.cpp:1067-1103
+ do {
+ // We only handle integral cast, when all the types are integrals.
+ // Otherwise, pass the expression to VisitSymExpr.
+ QualType T = RootSym->getType();
+ if (!T->isIntegralOrEnumerationType())
+ return VisitSymExpr(Sym);
+
----------------
I think all this extra logic about how we infer ranges for casts is
interesting, but should be a separate patch.
For now, you can simply put `return Visit(Sym->getOperand());`.
First, it will unblock you from depending on that `RangeFactory` feature.
And also have quite a few questions about this particular implementation, so it
will stagger this patch as well.
================
Comment at: clang/lib/StaticAnalyzer/Core/RangeConstraintManager.cpp:2464-2523
+std::tuple<ProgramStateRef, SymbolRef, RangeSet>
+RangeConstraintManager::handleSymbolCast(ProgramStateRef State, SymbolRef Sym,
+ RangeSet R) {
+ QualType T = Sym->getType();
+ if (!T->isIntegralOrEnumerationType() || R.isEmpty())
+ return {State, Sym, R};
+
----------------
I need more explanation why we have this function and why we call it where we
call it. Additionally, it again looks like it belongs in a separate patch.
CHANGES SINCE LAST ACTION
https://reviews.llvm.org/D103096/new/
https://reviews.llvm.org/D103096
_______________________________________________
cfe-commits mailing list
[email protected]
https://lists.llvm.org/cgi-bin/mailman/listinfo/cfe-commits
