ldionne added a comment.

In D136533#3896536 <https://reviews.llvm.org/D136533#3896536>, @mizvekov wrote:

> Now that you mention it, looking at the code, I think we don't diagnose an 
> use of a type alias itself, if that is what you mean?
>
> Ie, clang doesn't, GCC does, MSVC doesn't: https://godbolt.org/z/WEo4enjbz
>
> Would fixing that be a problem for libc++?

It might be a problem, but I would argue we should still do it after fixing any 
problematic cases. It seems like Clang's current behavior is broken, as it 
basically ignores the `[[deprecated]]` attribute on aliases?

> Otherwise, typename accesses through the type alias pattern itself should 
> work like from any context, except that if the access happens through a 
> dependent entity, we should be diagnosing it when we instantiate the type 
> alias.
>
> Does what you are talking about fall into any of that? Otherwise, do you have 
> a short example?

Sorry, I'm a bit lost. Let's take it back to the errors we're actually seeing 
with my reproducer above:

  In file included from <stdin>:1:
  In file included from SDK/usr/include/c++/v1/filesystem:245:
  In file included from 
SDK/usr/include/c++/v1/__filesystem/directory_entry.h:14:
  In file included from SDK/usr/include/c++/v1/__chrono/time_point.h:13:
  In file included from SDK/usr/include/c++/v1/__chrono/duration.h:14:
  In file included from SDK/usr/include/c++/v1/limits:107:
  In file included from SDK/usr/include/c++/v1/type_traits:421:
  In file included from SDK/usr/include/c++/v1/__functional/invoke.h:17:
  In file included from SDK/usr/include/c++/v1/__type_traits/decay.h:13:
  SDK/usr/include/c++/v1/__type_traits/add_pointer.h:26:50: error: 'type' is 
unavailable: introduced in macOS 10.15
                  _IsSame<typename remove_cv<_Tp>::type, void>::value>
                                                   ^
  SDK/usr/include/c++/v1/__type_traits/add_pointer.h:33:39: note: in 
instantiation of default argument for 
'__add_pointer_impl<std::filesystem::file_status>' required here
      {typedef _LIBCPP_NODEBUG typename __add_pointer_impl<_Tp>::type type;};
                                        ^~~~~~~~~~~~~~~~~~~~~~~
  SDK/usr/include/c++/v1/__type_traits/decay.h:44:40: note: in instantiation of 
template class 'std::add_pointer<std::filesystem::file_status>' requested here
                                typename add_pointer<_Up>::type,
                                         ^
  SDK/usr/include/c++/v1/__type_traits/decay.h:56:38: note: in instantiation of 
template class 'std::__decay<std::filesystem::file_status, true>' requested here
      typedef _LIBCPP_NODEBUG typename __decay<_Up, 
__is_referenceable<_Up>::value>::type type;
                                       ^
  SDK/usr/include/c++/v1/__filesystem/path.h:142:47: note: in instantiation of 
template class 'std::decay<std::filesystem::file_status>' requested here
  template <class _Source, class _DS = typename decay<_Source>::type,
                                                ^
  SDK/usr/include/c++/v1/__filesystem/path.h:195:31: note: in instantiation of 
default argument for '__is_pathable_char_array<std::filesystem::file_status>' 
required here
            bool _IsCharIterT = __is_pathable_char_array<_Tp>::value,
                                ^~~~~~~~~~~~~~~~~~~~~~~~~~~~~
  SDK/usr/include/c++/v1/__filesystem/path.h:445:26: note: in instantiation of 
default argument for '__is_pathable<std::filesystem::file_status, false>' 
required here
        typename enable_if<__is_pathable<_SourceOrIter>::value, _Tp>::type;
                           ^~~~~~~~~~~~~~~~~~~~~~~~~~~~
  SDK/usr/include/c++/v1/__filesystem/path.h:480:36: note: in instantiation of 
template type alias '_EnableIfPathable' requested here
    template <class _Source, class = _EnableIfPathable<_Source, void> >
                                     ^
  SDK/usr/include/c++/v1/__filesystem/path.h:482:3: note: in instantiation of 
default argument for 'path<std::filesystem::file_status>' required here
    path(const _Source& __src, format = format::auto_format) {
    ^~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~
  SDK/usr/include/c++/v1/__filesystem/operations.h:99:75: note: while 
substituting deduced template arguments into function template 'path' [with 
_Source = file_status, $1 = (no value)]
  inline _LIBCPP_HIDE_FROM_ABI bool exists(const path& __p) { return 
exists(__status(__p)); }
                                                                            ^
  SDK/usr/include/c++/v1/__filesystem/file_status.h:28:24: note: 'file_status' 
has been explicitly marked unavailable here
  class _LIBCPP_TYPE_VIS file_status {
                         ^

`file_status`, `exists` and `path::path` are all marked as unavailable. Why do 
we diagnose the use of an unavailable type wayyy lower in the stack when we 
instantiate `remove_cv`? I am not seeing what libc++ is doing wrong.

> Otherwise, if current libc++ is fine and building older libc++with newer 
> clang is not supported, is everyone fine with merging this back, as is?
> @thakis ?

See below for what I meant by "unsupported". Another way to think about this 
would be to say that Clang has the burden of keeping a slightly-older libc++ 
compiling. If that's the case, then one could argue that this change should be 
held off until Clang no longer cares about being able to compile that version 
of libc++. This is not really my decision to make, but I would argue that 
there's probably value in keeping things working (that would bite people not 
only on OS X, but anyone using an older libc++ with a newer Clang).

If so, then perhaps it would make sense to wait a few months before we merge 
this, perhaps at least until LLVM 16 is out so there's an officially-released 
version of libc++ that doesn't break with this change. But I think that's a 
decision for the Clang folks to make, not libc++.

In D136533#3897516 <https://reviews.llvm.org/D136533#3897516>, @glandium wrote:

>> you're using the SDK libc++ with a ToT clang, which is technically not a 
>> supported combination.
>
> Where is it specified that it's not a supported combination? Why should it 
> not be supported? You don't even get libc++ from the llvm tree unless you 
> explicitly enable it.

I guess I should reword it that way: If you're using an older libc++ with a 
newer Clang, there is literally no way for that old libc++ to guarantee that it 
can work with that not-known-yet Clang. It would imply being forward compatible 
with potentially arbitrary changes in Clang, which is nonsensical. IOW, we 
can't guard against issues that we don't know exist yet. Does that make sense?


Repository:
  rG LLVM Github Monorepo

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

https://reviews.llvm.org/D136533

_______________________________________________
cfe-commits mailing list
cfe-commits@lists.llvm.org
https://lists.llvm.org/cgi-bin/mailman/listinfo/cfe-commits

Reply via email to