[libcxx-commits] [PATCH] D119964: [libc++] Don't warn that coroutines aren't supported when including <experimental/coroutine>

[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
----------------
ldionne wrote:
> ChuanqiXu wrote:
> > philnik wrote:
> > > 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.
> > Personally, I prefer to diagnostic early if we detected user are doing the wrong behavior. But given that consistency is very important in projects like libcxx, I agree we should follow the usual convention to make it a no-op.
> > 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.
> 
> The note I have locally says LLVM 16. We shipped `<coroutine>` in LLVM 14, which means that we remove `<experimental/coroutines>` in LLVM 16. Please correct me if I'm mistaken.
As discussed on Discord I thought it was LLVM 15 and added that version in the release notes. The release notes have been updated in D120935.


Repository:
  rG LLVM Github Monorepo

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

https://reviews.llvm.org/D119964