[PATCH] D47603: [analyzer] fix bug with 1-bit APSInt types in Z3ConstraintManager

[George Karpenkov via Phabricator via cfe-commits]

george.karpenkov requested changes to this revision.
george.karpenkov added a comment.
This revision now requires changes to proceed.

Thanks! Looks good with minor changes.

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?



================
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;
----------------
What does `L` stand for here? It's confusing because `L/R` usually stand for left/right-hand-side in this context.


================
Comment at: lib/StaticAnalyzer/Core/Z3ConstraintManager.cpp:1154
+  BasicValueFactory &BVF = getBasicVals();
+  ASTContext &Ctx = BVF.getContext();
 
----------------
that's a separate change, but OK


================
Comment at: lib/StaticAnalyzer/Core/Z3ConstraintManager.cpp:1426
+    *Ty = getAPSIntType(Int);
+  return Int;
+}
----------------
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>` ?


================
Comment at: lib/StaticAnalyzer/Core/Z3ConstraintManager.cpp:1640
 #else
-  llvm::report_fatal_error("Clang was not compiled with Z3 support!", false);
+  llvm::report_fatal_error("Clang was not compiled with Z3 support, rebuild "
+                           "with -DCLANG_ANALYZER_BUILD_Z3=ON",
----------------
that's a separate change, but OK


Repository:
  rC Clang

https://reviews.llvm.org/D47603