[PATCH] D71612: [analyzer] Add PlacementNewChecker

[Gábor Horváth via Phabricator via cfe-commits]

xazax.hun added inline comments.


================
Comment at: clang/include/clang/StaticAnalyzer/Checkers/Checkers.td:472
+def PlacementNewChecker : Checker<"PlacementNew">,
+  HelpText<"Check if default placement new is provided with pointers to "
+           "sufficient storage capacity">,
----------------
Probably you want to add documentation to `clang/docs/analyzer/checkers.rst` as well for better visibility.


================
Comment at: clang/lib/StaticAnalyzer/Checkers/CheckPlacementNew.cpp:51
+SVal PlacementNewChecker::getExtentSizeOfNewTarget(
+    CheckerContext &C, const CXXNewExpr *NE, ProgramStateRef State) const {
+  SValBuilder &SvalBuilder = C.getSValBuilder();
----------------
A very minor nit, but checker APIs tend to have `CheckerContext` as the last parameter. Maybe following this guideline within the checkers as well makes them a bit more natural.


================
Comment at: clang/lib/StaticAnalyzer/Checkers/CheckPlacementNew.cpp:59
+    SVal ElementCount = C.getSVal(SizeExpr);
+    Optional<NonLoc> ElementCountNL = ElementCount.getAs<NonLoc>();
+    if (ElementCountNL) {
----------------
You can move this line into the if condition.


================
Comment at: clang/lib/StaticAnalyzer/Checkers/CheckPlacementNew.cpp:91
+  SVal SizeOfPlace = getExtentSizeOfPlace(C, Place, State);
+  const auto SizeOfTargetCI = SizeOfTarget.getAs<nonloc::ConcreteInt>();
+  if (!SizeOfTargetCI)
----------------
Here, instead of getting `SizeOfTarget` and `SizeOfPlace` as `ConcreteInt`s, I think you should rather use `evalBinOp` to compare them. That method is more future proof as if we cannot constraint these values down to a single integer but we still have some information about them a sufficiently smart solver could prove the relationship between the symbolic values.


Repository:
  rG LLVM Github Monorepo

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

https://reviews.llvm.org/D71612