[libcxx-commits] [PATCH] D106124: [libcxx][modules] protects users from relying on detail headers

[Louis Dionne via Phabricator via libcxx-commits]

ldionne added a comment.

This patch seems to break anyone using `-I <include>/c++/v1` (as opposed to `-isystem <include>/c++/v1`, the more usual incantation). For example, I'm seeing an error when building libc++abi, which boils down to:

  clang++ -nostdinc++ -I <build>/include/c++/v1 -std=c++2a libcxxabi/src/cxa_demangle.cpp -fsyntax-only



  In file included from libcxxabi/src/cxa_demangle.cpp:13:
  In file included from libcxxabi/src/demangle/ItaniumDemangle.h:25:
  In file included from libcxxabi/src/demangle/Utility.h:20:
  In file included from <build>/include/c++/v1/array:111:
  In file included from <build>/include/c++/v1/__algorithm/equal.h:15:
  In file included from <build>/include/c++/v1/__iterator/distance.h:14:
  In file included from <build>/include/c++/v1/__iterator/concepts.h:14:
  In file included from <build>/include/c++/v1/__iterator/incrementable_traits.h:14:
  In file included from <build>/include/c++/v1/concepts:133:
  In file included from <build>/include/c++/v1/__concepts/assignable.h:12:
  In file included from <build>/include/c++/v1/__concepts/common_reference_with.h:12:
  In file included from <build>/include/c++/v1/__concepts/convertible_to.h:14:
  <build>/include/c++/v1/type_traits:4104:10: error: header '<__utility/forward.h>' is an implementation detail; #include '<utility>' instead
  #include <__utility/forward.h>
           ^
  <build>/include/c++/v1/type_traits:4105:10: error: header '<__utility/move.h>' is an implementation detail; #include '<utility>' instead
  #include <__utility/move.h>
           ^

Reducing even more, it looks like the following breaks:

  echo "#include <algorithm>" | clang++ -nostdinc++ -I <build>/include/c++/v1 -fsyntax-only -xc++ -

I'm not sure why CI seems happy with the patch, but I definitely see those issues locally. I'm still investigating.

In D106124#3346614 <https://reviews.llvm.org/D106124#3346614>, @EricWF wrote:

> In D106124#3346349 <https://reviews.llvm.org/D106124#3346349>, @Quuxplusone wrote:
>
>> @EricWF obviously this should wait for @ldionne to return; it hasn't been touched since July 2021, so let's not jump the gun here.
>
> I don't see the need for that.

I'm a bit disappointed this was landed hastily -- there are some issues with this patch. Except for what I reported above, those are not major issues, but it would have been easy to address them before landing. For example, we don't support some of the compilers that this patch contains `UNSUPPORTED` markup for. I also share other reviewer's questions about the introduction of a new `threading_support.h` header inside libc++abi.

Anyway, I'm not really sure where to go from here, given that it breaks a pretty basic use case. I'll investigate this a bit more, but we might have to revert this until we figure out these issues.

Other than that, I like this patch, I think it provides a nice guard against people relying on our implementation details.


Repository:
  rG LLVM Github Monorepo

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

https://reviews.llvm.org/D106124