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