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

[Mark de Wever via Phabricator via libcxx-commits]

Mordante 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
----------------
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.




Repository:
  rG LLVM Github Monorepo

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

https://reviews.llvm.org/D119964