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

[Balázs Kéri via Phabricator via cfe-commits]

balazske marked 3 inline comments as done.
balazske added a comment.

C++17 makes things more difficult because the align is probably handled by `operator new`, probably not, depending on the defined allocation functions. This can be observed only with a non clang-tidy checker (we could compute the used alignment?). Probably the CERT rule is from the time before C++17. It looks like that by default the alignment is handled correctly in C++17 so the whole check is out of scope for that language version.



================
Comment at: clang-tools-extra/clang-tidy/cert/DefaultOperatorNewCheck.cpp:26
+
+  if (NewExpr->getNumPlacementArgs() > 0)
+    return;
----------------
martong wrote:
> 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?
Placement new provides already a memory location that is specified by the user (or some custom allocation function) so this is not a default case for which the warning should be provided.


================
Comment at: clang-tools-extra/clang-tidy/cert/DefaultOperatorNewCheck.cpp:41
+  unsigned SpecifiedAlignment = D->getMaxAlignment();
+  unsigned DefaultAlignment = Context.getTargetInfo().getCharAlign();
+  if (!SpecifiedAlignment)
----------------
martong wrote:
> 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.
I wanted to use `Context.getTargetInfo().getNewAlign()` here, is it correct? (Or `getNewAlign()/getCharWidth()`?)


================
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>
----------------
martong wrote:
> Why do we have these changes? Seems to be unrelated.
I used the `add_new_check.py` script, do not know why these changes appeared. (Probably issue with the diff or rebase?)


Repository:
  rG LLVM Github Monorepo

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

https://reviews.llvm.org/D67545