[libcxx-commits] [PATCH] D46443: [libc++] Add missing cstdalign header

[Louis Dionne via Phabricator via libcxx-commits]

ldionne requested changes to this revision.
ldionne added a comment.

Trying to summarize the discussion here for the author:

1. Please add a `<stdalign.h>` header to libc++ that does `#include_next <stdalign.h>` 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 <stdalign.h>
+
----------------
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 `<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?
> `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 <stdalign.h> are the same as the C standard library header <stdalign.h>, with the following changes: The header <stdalign.h> 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 `<stdalign.h>` 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