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

[Gabor Marton via Phabricator via cfe-commits]

martong added inline comments.


================
Comment at: clang-tools-extra/clang-tidy/cert/DefaultOperatorNewCheck.cpp:26
+
+  if (NewExpr->getNumPlacementArgs() > 0)
+    return;
----------------
Perhaps we should add in the docs that placement new is not supported. Or add a fixme here.
Anyway, I feel this code simply could work with placement new as well. What is the reason you disabled it for placement new?


================
Comment at: clang-tools-extra/clang-tidy/cert/DefaultOperatorNewCheck.cpp:41
+  unsigned SpecifiedAlignment = D->getMaxAlignment();
+  unsigned DefaultAlignment = Context.getTargetInfo().getCharAlign();
+  if (!SpecifiedAlignment)
----------------
This might not be what we want... `getCharAlign()` theoretically could return even with `1`, I think, though it would be a very strange architecture.
Perhaps we should use `getSuitableAlign()` instead?

Also, I think we should call the variable as `FundamentalAlignment`. Fundamental and default alignment can differ, I guess.


================
Comment at: clang-tools-extra/clang-tidy/cert/DefaultOperatorNewCheck.cpp:49
+  if (HasDefaultOperatorNew && OverAligned)
+    diag(NewExpr->getBeginLoc(), "using default 'operator new' with over-aligned type %0 may result in undefined behavior")
+      << D;
----------------
I think it would be useful to add the value of the fundamental alignment to the diagnostics too.


================
Comment at: clang-tools-extra/docs/clang-tidy/checks/list.rst:107
    cert-oop54-cpp (redirects to bugprone-unhandled-self-assignment) <cert-oop54-cpp>
-   clang-analyzer-core.CallAndMessage
-   clang-analyzer-core.DivideZero
+   clang-analyzer-core.CallAndMessage (redirects to https://clang.llvm.org/docs/analyzer/checkers) <clang-analyzer-core.CallAndMessage>
+   clang-analyzer-core.DivideZero (redirects to https://clang.llvm.org/docs/analyzer/checkers) <clang-analyzer-core.DivideZero>
----------------
Why do we have these changes? Seems to be unrelated.


Repository:
  rG LLVM Github Monorepo

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

https://reviews.llvm.org/D67545