[PATCH] D46443: [libc++] Add missing cstdalign header
ldionne requested changes to this revision. ldionne added a comment. Trying to summarize the discussion here for the author: 1. Please add a `` header to libc++ that does `#include_next ` and then defines both `__alignof_is_defined` and `__alignas_is_defined`. 2. Please re-add a test checking for `__alignof_is_defined`, but guard it with `#if defined(_LIBCPP_VER)` so that we only test it on libc++ (since technically the Standard doesn't require it, even if it's a defect). 3. Please re-run `libcxx/utils/generate_header_tests.py` and make sure that creates no diff. Comment at: libcxx/include/cstdalign:24 +#include <__config> +#include + hubert.reinterpretcast wrote: > sbc100 wrote: > > 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 `` > > > > > > > (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? > `stdalign.h` is a header defined in C++17 like `stdbool.h` is one. libc++ has > a `stdbool.h`, so I think having a `stdalign.h` makes sense (since we know > that `stdalign.h` in some environments does not do what we want). However, http://eel.is/c++draft/depr.stdalign.h.syn#1 says: > The contents of the C++ header are the same as the C standard > library header , with the following changes: The header > does not define a macro named alignas. I think the most efficient way of ensuring that is to use `#include_next`, which requires a C++ friendly `` on the platform. Generally speaking, I'm not in favour of trying to work around platforms that do not provide C++ friendly headers. If we're trying to make C++ work on those platforms, they need to minimally provide C++ friendly headers. 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
[PATCH] D46443: [libc++] Add missing cstdalign header
hubert.reinterpretcast added inline comments. Comment at: libcxx/include/cstdalign:24 +#include <__config> +#include + sbc100 wrote: > 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 `` > > > > > > (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? `stdalign.h` is a header defined in C++17 like `stdbool.h` is one. libc++ has a `stdbool.h`, so I think having a `stdalign.h` makes sense (since we know that `stdalign.h` in some environments does not do what we want). 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
[PATCH] D46443: [libc++] Add missing cstdalign header
sbc100 added inline comments. Comment at: libcxx/include/cstdalign:24 +#include <__config> +#include + 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 `` > > > > > (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
[PATCH] D46443: [libc++] Add missing cstdalign header
hubert.reinterpretcast added inline comments. Comment at: libcxx/include/cstdalign:24 +#include <__config> +#include + 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 `` > > > > (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`. 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
[PATCH] D46443: [libc++] Add missing cstdalign header
curdeius requested changes to this revision. curdeius added inline comments. This revision now requires changes to proceed. Comment at: libcxx/include/cstdalign:24 +#include <__config> +#include + 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 `` > > > (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. Comment at: libcxx/test/std/language.support/cstdalign/cstdalign.pass.cpp:16-18 +#ifdef alignas +#error alignas should not be a macro in C++ +#endif Please do the same for `alignof`. Comment at: libcxx/test/std/language.support/cstdalign/cstdalign.pass.cpp:27 +#ifndef __alignof_is_defined +#error __alignof_is_defined not defined +#endif hubert.reinterpretcast wrote: > sbc100 wrote: > > hubert.reinterpretcast wrote: > > > sbc100 wrote: > > > > ldionne wrote: > > > > > I'm not seeing `__alignof_is_defined` anywhere in the spec? > > > > Removed > > > Seems like a defect in the old standard. The prose doesn't match the > > > synopsis. `__alignof_is_defined` is a macro in C11's `stdalign.h` (and so > > > is `alignof`). That the C++ committee did not intend for an `alignof` > > > macro can probably be assumed. I suspect the lack of an > > > `__alignof_is_defined` macro was also unintended. > > So should I add back the check for `__alignof_is_defined`? > I think so (in addition to also checking that `alignof` is not defined as a > macro). I think the patch needs to be confirmed again either way by the > libc++ approvers though. Please check `__alignof_is_defined` as well, but guard it with `#ifdef _LIBCPP_VERSION` to be conforming. I agree that it seems to be a defect in the old standard. 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
[PATCH] D46443: [libc++] Add missing cstdalign header
hubert.reinterpretcast added inline comments. Comment at: libcxx/include/cstdalign:24 +#include <__config> +#include + 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 `` > > (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). 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
[PATCH] D46443: [libc++] Add missing cstdalign header
curdeius added inline comments. Comment at: libcxx/include/cstdalign:24 +#include <__config> +#include + 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 `` > (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. 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
[PATCH] D46443: [libc++] Add missing cstdalign header
curdeius added inline comments. Comment at: libcxx/include/cstdalign:24 +#include <__config> +#include + 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 `` (https://github.com/microsoft/STL/blob/main/stl/inc/cstdalign). @hubert.reinterpretcast, are you aware of an environment which has non-friendly `stdalign.h`? 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
[PATCH] D46443: [libc++] Add missing cstdalign header
hubert.reinterpretcast added inline comments. Comment at: libcxx/include/cstdalign:24 +#include <__config> +#include + 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`. Comment at: libcxx/test/std/language.support/cstdalign/cstdalign.pass.cpp:27 +#ifndef __alignof_is_defined +#error __alignof_is_defined not defined +#endif sbc100 wrote: > hubert.reinterpretcast wrote: > > sbc100 wrote: > > > ldionne wrote: > > > > I'm not seeing `__alignof_is_defined` anywhere in the spec? > > > Removed > > Seems like a defect in the old standard. The prose doesn't match the > > synopsis. `__alignof_is_defined` is a macro in C11's `stdalign.h` (and so > > is `alignof`). That the C++ committee did not intend for an `alignof` macro > > can probably be assumed. I suspect the lack of an `__alignof_is_defined` > > macro was also unintended. > So should I add back the check for `__alignof_is_defined`? I think so (in addition to also checking that `alignof` is not defined as a macro). I think the patch needs to be confirmed again either way by the libc++ approvers though. 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
[PATCH] D46443: [libc++] Add missing cstdalign header
sbc100 added inline comments. Comment at: libcxx/include/cstdalign:24 +#include <__config> +#include + 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? Comment at: libcxx/test/std/language.support/cstdalign/cstdalign.pass.cpp:27 +#ifndef __alignof_is_defined +#error __alignof_is_defined not defined +#endif hubert.reinterpretcast wrote: > sbc100 wrote: > > ldionne wrote: > > > I'm not seeing `__alignof_is_defined` anywhere in the spec? > > Removed > Seems like a defect in the old standard. The prose doesn't match the > synopsis. `__alignof_is_defined` is a macro in C11's `stdalign.h` (and so is > `alignof`). That the C++ committee did not intend for an `alignof` macro can > probably be assumed. I suspect the lack of an `__alignof_is_defined` macro > was also unintended. So should I add back the check for `__alignof_is_defined`? 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
[PATCH] D46443: [libc++] Add missing cstdalign header
hubert.reinterpretcast added inline comments. Comment at: libcxx/include/cstdalign:24 +#include <__config> +#include + 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. Comment at: libcxx/test/std/language.support/cstdalign/cstdalign.pass.cpp:27 +#ifndef __alignof_is_defined +#error __alignof_is_defined not defined +#endif sbc100 wrote: > ldionne wrote: > > I'm not seeing `__alignof_is_defined` anywhere in the spec? > Removed Seems like a defect in the old standard. The prose doesn't match the synopsis. `__alignof_is_defined` is a macro in C11's `stdalign.h` (and so is `alignof`). That the C++ committee did not intend for an `alignof` macro can probably be assumed. I suspect the lack of an `__alignof_is_defined` macro was also unintended. 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
[PATCH] D46443: [libc++] Add missing cstdalign header
ldionne accepted this revision. ldionne added a comment. This revision is now accepted and ready to land. Please wait for CI to finish! 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
[PATCH] D46443: [libc++] Add missing cstdalign header
curdeius accepted this revision as: curdeius. curdeius added a comment. LGTM but please wait for libc++ group approval. If you need someone to land it for you, please provide "Firstname Name " for attribution. 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
