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