[PATCH] D67545: [clang-tidy] Added DefaultOperatorNewCheck.

[Aaron Ballman via Phabricator via cfe-commits]

aaron.ballman added inline comments.
Herald added a subscriber: hiraditya.


================
Comment at: clang-tools-extra/clang-tidy/cert/CERTTidyModule.cpp:68-70
+    // MEM
+    CheckFactories.registerCheck<DefaultOperatorNewAlignmentCheck>(
+        "cert-mem57-cpp");
----------------
The `MEM` section should come before `MSC`.


================
Comment at: clang-tools-extra/clang-tidy/cert/DefaultOperatorNewAlignmentCheck.cpp:24
+void DefaultOperatorNewAlignmentCheck::check(
+    const MatchFinder::MatchResult &Result) {
+  // Get the found 'new' expression.
----------------
Didn't we decide the check should not diagnose in C++17 or later?


================
Comment at: clang-tools-extra/clang-tidy/cert/DefaultOperatorNewAlignmentCheck.cpp:29-30
+  // Skip placement new: This is a user-defined allocation.
+  if (NewExpr->getNumPlacementArgs() > 0)
+    return;
+  QualType T = NewExpr->getAllocatedType();
----------------
I think this should be hoisted into a local AST matcher rather than be part of the `check()` call. Something like `unless(isPlacementNew())` when registering the check.


================
Comment at: clang-tools-extra/clang-tidy/cert/DefaultOperatorNewAlignmentCheck.cpp:47
+  // The user-specified alignment (in bits).
+  const unsigned SpecifiedAlignment = D->getMaxAlignment();
+  // Double-check if no alignment was specified.
----------------
Please drop top-level `const` on anything that's not a pointer or a reference (that's not a style we typically follow).


================
Comment at: clang-tools-extra/clang-tidy/cert/DefaultOperatorNewAlignmentCheck.cpp:59
+  const unsigned CharWidth = Context.getTargetInfo().getCharWidth();
+  if (HasDefaultOperatorNew && OverAligned /*&& !NewExpr->passAlignment()*/)
+    diag(NewExpr->getBeginLoc(),
----------------
Commented-out code?


Repository:
  rG LLVM Github Monorepo

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

https://reviews.llvm.org/D67545