ddcc added a comment. In https://reviews.llvm.org/D47603#1118138, @vlad.tsyrklevich wrote:
> In https://reviews.llvm.org/D47603#1118106, @george.karpenkov wrote: > > > Would it be possible to add tests? I know we have very few unit tests, but > > I assume you could actually use an integration test to exercise this path? > > > I tested this change and it fixes PR37622. There's a simple crash reproducer > included there. Cool, thanks for the repro! It's been long enough since I've touched this code that I don't recall the original failing testcase. I'll add the test to this revision. ================ Comment at: lib/StaticAnalyzer/Core/Z3ConstraintManager.cpp:1044 Z3Expr Exp = getZ3Expr(Sym, &RetTy); - - assert((getAPSIntType(From) == getAPSIntType(To)) && - "Range values have different types!"); - QualType RTy = getAPSIntType(From); - bool isSignedTy = RetTy->isSignedIntegerOrEnumerationType(); - Z3Expr FromExp = Z3Expr::fromAPSInt(From); - Z3Expr ToExp = Z3Expr::fromAPSInt(To); + QualType LTy; + llvm::APSInt NewFromInt; ---------------- george.karpenkov wrote: > What does `L` stand for here? It's confusing because `L/R` usually stand for > left/right-hand-side in this context. They correspond to the inferred left/right hand-side inferred types, but inside the subsequent Z3Expr `LHS` and `RHS` variables. This is confusing, so I'll rename them to `FromTy` and `ToTy`. ================ Comment at: lib/StaticAnalyzer/Core/Z3ConstraintManager.cpp:1154 + BasicValueFactory &BVF = getBasicVals(); + ASTContext &Ctx = BVF.getContext(); ---------------- george.karpenkov wrote: > that's a separate change, but OK Yeah, this is one of several small miscellaneous changes that didn't make the original commit. It seemed a bit excessive to open separate revisions for each, so I've just been merging them into the next patch. I'm not sure which is preferable? ================ Comment at: lib/StaticAnalyzer/Core/Z3ConstraintManager.cpp:1426 + *Ty = getAPSIntType(Int); + return Int; +} ---------------- george.karpenkov wrote: > It's redundant to mutate the argument passed by reference and also return it. > Could we take a single `APSInt` parameter by value and return > `std::pair<APSInt, QualType>` ? Sure. Repository: rC Clang https://reviews.llvm.org/D47603 _______________________________________________ cfe-commits mailing list cfe-commits@lists.llvm.org http://lists.llvm.org/cgi-bin/mailman/listinfo/cfe-commits
