aaron.ballman added a comment. Thank you for working on this check!
In D67545#1672106 <https://reviews.llvm.org/D67545#1672106>, @balazske wrote: > C++17 makes things more difficult because the align is probably handled by > `operator new`, probably not, depending on the defined allocation functions. Correct. > This can be observed only with a non clang-tidy checker (we could compute the > used alignment?). I'm less certain of this. We should be able to look up which `operator new` overload was selected for the `new` expression and check to see if it has an alignment parameter. If it does, I think it's reasonable to assume that the function behaves correctly wrt 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. Yeah, the CERT C++ rules were written for C++14: https://wiki.sei.cmu.edu/confluence/display/cplusplus/Scope The check may be out of scope for C++17, I've not analyzed it for that. There are times when new with align is not called (http://eel.is/c++draft/cpp.predefined#1.6) and user overloads to consider as well, but I think those should be trying to call the aligned new version when given an over-aligned type. ================ Comment at: clang-tools-extra/clang-tidy/cert/CERTTidyModule.cpp:53 + CheckFactories.registerCheck<DefaultOperatorNewCheck>( + "cert-default-operator-new"); CheckFactories.registerCheck<performance::MoveConstructorInitCheck>( ---------------- balazske wrote: > The checker should be renamed to `cert-mem57-cpp` to comply with the others. Yes, please. It should also be moved under a `// MEM` comment instead of `OOP` (and kept in alphabetical order). ================ Comment at: clang-tools-extra/clang-tidy/cert/DefaultOperatorNewCheck.cpp:26 + + if (NewExpr->getNumPlacementArgs() > 0) + return; ---------------- balazske wrote: > 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. That would correspond to MEM54-CPP: https://wiki.sei.cmu.edu/confluence/display/cplusplus/MEM54-CPP.+Provide+placement+new+with+properly+aligned+pointers+to+sufficient+storage+capacity 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/DefaultOperatorNewCheck.cpp:41 + unsigned SpecifiedAlignment = D->getMaxAlignment(); + unsigned DefaultAlignment = Context.getTargetInfo().getCharAlign(); + if (!SpecifiedAlignment) ---------------- balazske wrote: > 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()`?) I think `getNewAlign()` is the correct thing to use here. If the alignment requirements of the allocated type are greater than what `getNewAlign()` returns, there are problems (unless calling the C++17 aligned allocation operator). ================ 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; ---------------- balazske wrote: > martong wrote: > > I think it would be useful to add the value of the fundamental alignment to > > the diagnostics too. > Should be this the correct error message? "Allocation with standard new: too > long alignment for a user type." Or "Allocation with standard new: too long > alignment (//user_align//) for a user type, default is //default_align//." I'd go with: `allocation function returns a pointer with alignment %0 but the over-aligned type being allocated requires alignment %1` ================ Comment at: clang-tools-extra/clang-tidy/cert/DefaultOperatorNewCheck.h:18 + +/// FIXME: Write a short description. +/// ---------------- FIXME needs fixing. ================ Comment at: clang-tools-extra/docs/ReleaseNotes.rst:79 + + FIXME: add release notes. + ---------------- You should fix this fixme. ================ Comment at: clang-tools-extra/docs/clang-tidy/checks/cert-default-operator-new.rst:6 + +FIXME: Describe what patterns does the check detect and why. Give examples. ---------------- This one too -- you should model it after the other CERT check documentation, with a link back to the CERT check. ================ 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> ---------------- balazske wrote: > 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?) Something got messed up -- you should back out these changes. 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
