[libcxx-commits] [PATCH] D119964: [libc++] Generated headers should first include <__config>

[Nikolas Klauser via Phabricator via libcxx-commits]

philnik added inline comments.


================
Comment at: libcxx/test/libcxx/clang_tidy.sh.cpp:208-212
+#    include <experimental/__config>
 #    include <experimental/algorithm>
 #    ifndef _LIBCPP_HAS_NO_EXPERIMENTAL_COROUTINES
 #        include <experimental/coroutine>
 #    endif
----------------
Mordante wrote:
> Quuxplusone wrote:
> > >>! In D119964#3327193, @philnik wrote:
> > >>>! In D119964#3327177, @Quuxplusone wrote:
> > >> No, that definitely shouldn't be the case. There's no need to `#include <__config>` before `#include <algorithm>`, because `<algorithm>` itself includes `<__config>`.
> > >> (And there's no need for `<algorithm>` to include `<__config>` before including `<__algorithm/adjacent_find.h>`, because `<__algorithm/adjacent_find.h>` itself includes `<__config>`! And so on.)
> > > 
> > > The problem is that we use macros from `<__config>`. This means that we might include a header that shouldn't be included. Just as an example, //maybe// we want to remove `<experimental/algorithm>`. That means that the first include in the list is conditionally included, but the header where the condition is set isn't included before the condition is checked. That means `<experimental/coroutine>` is always included, but might not be available in a configuration.
> > 
> > Aha, I see. So there's no problem //today//, but there will be a problem as soon as you `git rm experimental/algorithm`, because then the first header in the list of experimental headers (alphabetically) will be one that it's not always safe to include.
> > 
> > I guess my next question is, why is it unsafe to unconditionally include `<experimental/coroutine>`? So I looked at the top of that file and found
> > ```
> > #ifdef _LIBCPP_HAS_NO_EXPERIMENTAL_COROUTINES
> > # if defined(_LIBCPP_WARNING)
> >     _LIBCPP_WARNING("<experimental/coroutine> cannot be used with this compiler")
> > # else
> > #   warning <experimental/coroutine> cannot be used with this compiler
> > # endif
> > #endif
> > ```
> > Perhaps we should replace this with our usual convention, which is just to make the whole file a no-op if it's not supported. (`<coroutine>` uses our usual convention, because it was written more recently.) @ldionne @ChuanqiXu, thoughts?
> > >>! In D119964#3327193, @philnik wrote:
> > >>>! In D119964#3327177, @Quuxplusone wrote:
> > Perhaps we should replace this with our usual convention, which is just to make the whole file a no-op if it's not supported. (`<coroutine>` uses our usual convention, because it was written more recently.) @ldionne @ChuanqiXu, thoughts?
> 
> In the release notes we mentioned `<experimental/coroutine>` will be removed in LLVM 15. So maybe removing this header now will be a better solution.
> 
> 
It would probably be a good idea. I'll put it on my TODO-list. I still think it's a good idea to add the `<__config>` includes to make sure we have all the macros before we check if they are defined.


Repository:
  rG LLVM Github Monorepo

CHANGES SINCE LAST ACTION
  https://reviews.llvm.org/D119964/new/

https://reviews.llvm.org/D119964