martong added inline comments.
================ Comment at: clang-tools-extra/clang-tidy/cert/DefaultOperatorNewCheck.cpp:51 + // The alignment used by default 'operator new' (in bits). + const unsigned DefaultAlignment = Context.getTargetInfo().getNewAlign(); + ---------------- martong wrote: > aaron.ballman wrote: > > lebedev.ri wrote: > > > martong wrote: > > > > martong wrote: > > > > > What is the difference between "default" and "fundamental" alignment? > > > > > Are they the same? Can they differ in any architecture? > > > > > > > > > > https://wiki.sei.cmu.edu/confluence/display/cplusplus/MEM57-CPP.+Avoid+using+default+operator+new+for+over-aligned+types > > > > > Here there is no wording of "default alignment" only "fundamental > > > > > alignment" is mentioned. Based on this I'd call this as > > > > > `FundamentalAligment`. > > > > > What is the difference between "default" and "fundamental" alignment? > > > > > Are they the same? > > > > > > > > `fundamental alignment` of any type is the alignment of > > > > std::max_align_t. I.e. `alignof(std::max_align_t)`. > > > > See C++17 6.11.2. > > > > > > > > On the other hand, default alignment is the value in > > > > `__STDCPP_DEFAULT_NEW_ALIGNMENT__` which may be predefined with > > > > `fnew-alignment` > > > > See https://www.bfilipek.com/2019/08/newnew-align.html > > > > > > > > These values can differ: https://wandbox.org/permlink/yIwjiNMw9KyXEQan > > > > > > > > Thus, I think we should use the fundamental alignment here, not the > > > > default alignment. > > > > So, `getNewAlign()` does not seem right to me. > > > > @aaron.ballman What do you think? > > > > Thus, I think we should use the fundamental alignment here, not the > > > > default alignment. > > > > > > I have the exact opposite view. > > > If as per `getNewAlign()` the alignment would be okay, why should we not > > > trust it? > > The comment on `getNewAlign()` is: > > ``` > > /// Return the largest alignment for which a suitably-sized allocation > > with > > /// '::operator new(size_t)' is guaranteed to produce a correctly-aligned > > /// pointer. > > ``` > > I read that as saying any alignment larger than what is returned by > > `getNewAlign()` must call the over-aligned operator new variant in C++17 if > > available. So if the actual call target doesn't have an alignment > > specifier, it's probably getting the alignment wrong and would be worth > > diagnosing on. > > I have the exact opposite view. > > If as per getNewAlign() the alignment would be okay, why should we not > > trust it? > > That could lead to a false positive diagnostic if `-fnew-alignment=8` and > `alignas(16)` , because `alignof(max_align_t)` is still 16. > > See the definidion of `getNewAlign()` which will return with 8 in this case I > suppose: > ``` > unsigned getNewAlign() const { > return NewAlign ? NewAlign : std::max(LongDoubleAlign, LongLongAlign); > } > ``` > So if the actual call target doesn't have an alignment specifier, it's > probably getting the alignment wrong and would be worth diagnosing on. I agree, but then we are implementing a checker which is different from the description given in cert-mem57. So it is not a CERT checker anymore, perhaps we should rename then. There is no mention of __STDCPP_DEFAULT_NEW_ALIGNMENT__ in https://wiki.sei.cmu.edu/confluence/display/cplusplus/MEM57-CPP.+Avoid+using+default+operator+new+for+over-aligned+types It clearly references the "fundamental alignement". 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
