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
_______________________________________________
cfe-commits mailing list
cfe-commits@lists.llvm.org
https://lists.llvm.org/cgi-bin/mailman/listinfo/cfe-commits
