[PATCH] D60139: [clang-tidy] Add bugprone-placement-new-target-type-mismatch check

[Aaron Ballman via Phabricator via cfe-commits]

aaron.ballman added a comment.

In D60139#1461269 <https://reviews.llvm.org/D60139#1461269>, @DennisL wrote:

> In D60139#1460233 <https://reviews.llvm.org/D60139#1460233>, @JonasToth wrote:
>
> > Hey Dennis,
> >
> > my 2cents on the check. I think it is very good to have! Did you check coding guidelines if they say something to this issue? (e.g. cppcoreguidelines, hicpp, cert) As we have modules for them it would be great to make aliases to this check if they demand this to be checked.
>
>
> Thanks for the great suggestions. Updated the diff according to the feedback. Also checked with cppcoreguidelines, hicpp as well as cert. Only cert has a related, yet different rule <https://wiki.sei.cmu.edu/confluence/display/cplusplus/MEM54-CPP.+Provide+placement+new+with+properly+aligned+pointers+to+sufficient+storage+capacity> stating that calls to placement new shall be provided with properly aligned pointers. I'd say this should be a distinct check. Happy to work on it after this one.


What is the rationale for separating those checks? It sort of feels like there is some overlap between alignment, size, and buffer type.

I'm not certain I agree with the way this check operates -- what problem is it trying to solve? For instance, this code would trigger on this check, but is perfectly valid code:

  static_assert(sizeof(int) == sizeof(float));
  static_assert(alignof(int) == alignof(float));
  int buffer;
  float *fp = new (&buffer) float;

The situations I can think of where the type mismatch is an issue would be for types with nontrivial special members, so there are times when this check makes sense, but perhaps it's too restrictive currently, but you may have a different problem in mind?



================
Comment at: clang-tidy/bugprone/PlacementNewTargetTypeMismatch.cpp:20
+namespace {
+AST_MATCHER(Expr, isPlacementNewExpr) {
+  const auto *NewExpr = dyn_cast<CXXNewExpr>(&Node);
----------------
This should match on `CXXNewExpr` instead of `Expr`, then you won't need the `dyn_cast` below.


================
Comment at: clang-tidy/bugprone/PlacementNewTargetTypeMismatch.cpp:29
+  Finder->addMatcher(
+      expr(isPlacementNewExpr(), unless(hasDescendant(unresolvedLookupExpr())))
+          .bind("NewExpr"),
----------------
This should probably use `cxxNewExpr()` instead of `expr()` so you match on something more specific.


================
Comment at: clang-tidy/bugprone/PlacementNewTargetTypeMismatch.cpp:38
+  assert(NewExpr && "Matched node bound by 'NewExpr' shoud be a 'CXXNewExpr'");
+  assert(NewExpr->getNumPlacementArgs() != 0 && "");
+
----------------
This assert message looks a bit incomplete. ;-)


================
Comment at: clang-tidy/bugprone/PlacementNewTargetTypeMismatch.cpp:43
+  assert(PlacementExpr != nullptr && "PlacementExpr should not be null");
+  const CastExpr *Cast = dyn_cast<CastExpr>(PlacementExpr);
+  if (Cast == nullptr)
----------------
You can use `const auto *` here.


================
Comment at: test/clang-tidy/bugprone-placement-new-target-type-mismatch.cpp:56
+  new (ptr) float[13];
+  // CHECK-MESSAGES: :[[@LINE-1]]:8: warning: placement new parameter and allocated type mismatch [bugprone-placement-new-target-type-mismatch]
+}
----------------
While this code definitely has a bug from the buffer overflow, I don't think the diagnostic is valuable here. This is a very common pattern and I suspect this will yield a lot of false positives, unless the check starts taking alignment and buffer size into consideration.


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

https://reviews.llvm.org/D60139