sbc100 added inline comments.
================ Comment at: libcxx/include/cstdalign:24 +#include <__config> +#include <stdalign.h> + ---------------- hubert.reinterpretcast wrote: > curdeius wrote: > > hubert.reinterpretcast wrote: > > > curdeius wrote: > > > > curdeius wrote: > > > > > hubert.reinterpretcast wrote: > > > > > > sbc100 wrote: > > > > > > > hubert.reinterpretcast wrote: > > > > > > > > This seems to be assuming that the underlying C library's > > > > > > > > `stdalign.h` is C++ friendly. A C11 `stdalign.h` //does// > > > > > > > > define `alignof` and `alignas` as macros. > > > > > > > Should I just remove this `#include` then? > > > > > > The idea would be to //add// a `stdalign.h` alongside this header > > > > > > that doesn't `#include_next` the underlying C library's > > > > > > `stdalign.h`. > > > > > I'm not sure if that should be the solution. At least gcc's libstdc++ > > > > > assumes that `stdalign.h` is C++-compatbile (cf. > > > > > https://github.com/gcc-mirror/gcc/blob/16e2427f50c208dfe07d07f18009969502c25dc8/libstdc%2B%2B-v3/include/c_global/cstdalign). > > > > > > > > > > Clang provides a compatible header: > > > > > https://github.com/llvm/llvm-project/commit/8acb4044d83ecc9df81b1c9f327d5bd4325e1756. > > > > > Gcc too of course: > > > > > https://github.com/gcc-mirror/gcc/blob/16e2427f50c208dfe07d07f18009969502c25dc8/gcc/ginclude/stdalign.h. > > > > > > > > > > MSVC's STL on the other hand, doesn't include `<stdalign.h>` > > > > > (https://github.com/microsoft/STL/blob/main/stl/inc/cstdalign). > > > > > > > > > > @hubert.reinterpretcast, are you aware of an environment which has > > > > > non-friendly `stdalign.h`? > > > > FYI, musl is also C++ friendly: > > > > https://git.musl-libc.org/cgit/musl/tree/include/stdalign.h. > > > >>! Quote: > > > > @hubert.reinterpretcast, are you aware of an environment which has > > > > non-friendly stdalign.h? > > > > > > The one GCC provides disagrees with the interpretation I gave of which > > > macros should be present. The one that Clang provides //does// match my > > > interpretation. It seems the GCC one is non-friendly (albeit a different > > > form of non-friendly than the one I opened with). > > Oh, you mean that `__alignas_is_defined` and `__alignof_is_defined` won't > > be defined in this case, right? > > In this case, I guess we won't avoid having `stdalign.h` as you had > > suggested. > > And indeed the test fails with gcc: > > ``` > > bin/llvm-lit -vv ../../libcxx/test/std/language.support/cstdalign/ > > --param=std=c++17 --param=cxx_under_test=`which g++` > > ... > > libcxx/test/std/language.support/cstdalign/cstdalign.pass.cpp:21:2: error: > > #error __alignas_is_defined not defined > > 21 | #error __alignas_is_defined not defined > > | ^~~~~ > > ``` > > > > That's unfortunately a configuration which is not tested in the CI. > Yes, it looks like adding a `stdalign.h` for libc++ is needed to reliably get > `__alignas_is_defined` and `__alignof_is_defined`. Alternatively, could we just defined them if they are not already and skip including `stdalign.h` here? Repository: rG LLVM Github Monorepo CHANGES SINCE LAST ACTION https://reviews.llvm.org/D46443/new/ https://reviews.llvm.org/D46443 _______________________________________________ cfe-commits mailing list cfe-commits@lists.llvm.org https://lists.llvm.org/cgi-bin/mailman/listinfo/cfe-commits
