[PATCH] D76996: [analyzer] Improve PlacementNewChecker

[Gabor Marton via Phabricator via cfe-commits]

martong added inline comments.


================
Comment at: clang/lib/StaticAnalyzer/Checkers/CheckPlacementNew.cpp:261
+
+  if (const VarRegion *TheVarRegion = BaseRegion->getAs<VarRegion>()) {
+    const VarDecl *TheVarDecl = TheVarRegion->getDecl();
----------------
Perhaps you could call instead `checkVarRegionAlign()`?


================
Comment at: clang/lib/StaticAnalyzer/Checkers/CheckPlacementNew.cpp:261
+
+  if (const VarRegion *TheVarRegion = BaseRegion->getAs<VarRegion>()) {
+    const VarDecl *TheVarDecl = TheVarRegion->getDecl();
----------------
martong wrote:
> Perhaps you could call instead `checkVarRegionAlign()`?
Also, I think `BaseRegion` can be an `ElementRegion` here as well. So, in that case we should call into `checkElementRegionAlign()`, shouldn't we? This draws a pattern that we should recursively descend down to the top most base region. I.e. the different `check*RegionAlign` methods should call into each other until we reach the top level base region.

The observation here is that the alignment of a region can be correct only if we can prove that its base region is aligned properly (and other requirements, e.g. the offset is divisible). But the base region may have another base region and we have to prove the alignment correctness to that as well.

I hope this makes sense, please correct me if I am wrong.


================
Comment at: clang/test/Analysis/placement-new.cpp:245
+
+  // ok. 2(short align) + 3*2(index '3' offset)
+  ::new (&b[3]) long;
----------------
So these tests failed after you rewrote ElementRegion processing, right?
Actually, I wonder why we thought that if x is divisible by 2 then (x+6) will be divisible by 8 unconditionally.It's good you have this fixed.


Repository:
  rG LLVM Github Monorepo

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

https://reviews.llvm.org/D76996