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

Jonas Toth via Phabricator via cfe-commits cfe-commits at lists.llvm.org
Tue Apr 9 11:43:48 PDT 2019


JonasToth added a comment.

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.



================
Comment at: clang-tidy/bugprone/PlacementNewTargetTypeMismatch.cpp:21
+void PlacementNewTargetTypeMismatch::registerMatchers(MatchFinder *Finder) {
+  // We only want the records that call 'new' with an adress parameter
+  Finder->addMatcher(cxxNewExpr(hasDescendant(castExpr())).bind("NewExpr"), this);
----------------
Please write full sentences with punctuation in comments.


================
Comment at: clang-tidy/bugprone/PlacementNewTargetTypeMismatch.cpp:22
+  // We only want the records that call 'new' with an adress parameter
+  Finder->addMatcher(cxxNewExpr(hasDescendant(castExpr())).bind("NewExpr"), this);
+}
----------------
placement-new? please make a new matcher (locally) to match that, because its is more specific.
You can use other checks as example, grep for "AST_MATCHER"


================
Comment at: clang-tidy/bugprone/PlacementNewTargetTypeMismatch.cpp:30
+
+  if (0 == NewExpr->getNumPlacementArgs()) {
+    return;
----------------
Please ellide braces.


================
Comment at: clang-tidy/bugprone/PlacementNewTargetTypeMismatch.cpp:38
+  const CastExpr *Cast = dyn_cast<CastExpr>(PlacementExpr);
+  if (nullptr == Cast) {
+    return;
----------------
braces, above the comment does not follow the correct-sentence style


================
Comment at: clang-tidy/bugprone/PlacementNewTargetTypeMismatch.cpp:42
+
+  assert((Cast->getSubExpr()->getType()->isPointerType() ||
+         Cast->getSubExpr()->getType()->isArrayType()) &&
----------------
Is this universally true? What about the nothrow-overload, would that interfere?


================
Comment at: clang-tidy/bugprone/PlacementNewTargetTypeMismatch.cpp:45
+         "Cast of placement parameter requires a pointer or an array type");
+  const QualType &PlacementParameterType =
+      Cast->getSubExpr()->getType()->getPointeeOrArrayElementType()->getCanonicalTypeInternal();
----------------
`QualType` is usually used a value-type, not const-ref. Please follow that for consistency.


================
Comment at: clang-tidy/bugprone/PlacementNewTargetTypeMismatch.cpp:50
+
+  if (PlacementParameterType != AllocatedType) {
+    diag(PlacementExpr->getExprLoc(), "placement new parameter and allocated type mismatch");
----------------
braces.


================
Comment at: docs/clang-tidy/checks/bugprone-placement-new-target-type-mismatch.rst:7
+Finds placement-new calls where the pointer type of the adress mismatches the
+type of the created value.
+
----------------
Please add examples to demonstrate good and bad code for the user.


================
Comment at: test/clang-tidy/bugprone-placement-new-target-type-mismatch.cpp:5
+
+using size_type = unsigned long;
+void *operator new(size_type, void *);
----------------
Please add a test for the nothrow overload.


================
Comment at: test/clang-tidy/bugprone-placement-new-target-type-mismatch.cpp:86
+  new (array) char('A');
+}
----------------
Please add test, where everything is hidden behind templates and only the type-substitution leads to the error.
A test including macros helps as well, to show they do not interfere (as should be the case with this check).


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

https://reviews.llvm.org/D60139





More information about the cfe-commits mailing list