This is an archive of the discontinued LLVM Phabricator instance.

[libc++] Mark <filesystem> as unavailable on Apple platforms
AbandonedPublic

Authored by ldionne on Mar 7 2019, 7:03 AM.

Details

Summary

Also add the corresponding XFAILs to tests that require filesystem.
Note that I did try to use a less intrusive approach using pragma
clang attribute, but this didn't work due to apparent bugs in Clang
(which should be fixed independently but should not delay this change).

Also note that we mark the functions as being introduced in versions
99.99 of the platforms because of a Clang bug (PR40991), which causes
a hard error when an unavailable function is used to initialize the
default argument of another unavailable function. We run into this
issue with functions like proximate().

Event Timeline

ldionne created this revision.Mar 7 2019, 7:03 AM
dexonsmith added inline comments.Mar 7 2019, 10:24 AM
libcxx/include/__config
1372–1375

Can we make this 9999?

I don't like how intrusive and pervasive this is.

libcxx/include/chrono
2830

This is a ABI change for people who are using the filesystem.
A public symbol has become non-public.

ldionne marked an inline comment as done.Mar 7 2019, 10:38 AM

I don't like how intrusive and pervasive this is.

Me neither. Per our offline conversation, I'm currently trying to get the #pragma approach to work (again). I'll report here.

libcxx/include/chrono
2830

Sorry, I don't follow you here. How did I change the symbol?

mclow.lists added inline comments.Mar 7 2019, 10:50 AM
libcxx/include/chrono
2830

Sorry; my bad. Misleading diff markup.

jfb added a subscriber: jfb.Mar 8 2019, 1:45 PM
EricWF added a comment.Mar 8 2019, 7:38 PM

We've got to find a better way to do this.

Also, a lot of these symbols are inline and don't depend on anything in the dylib. Why are the being marked unavailable?

We've got to find a better way to do this.

Yes, I don't like this but it is necessary if we want to ship <filesystem> as part of the dylib. Like I said, it is possible to use #pragma clang attribute to apply this to several declarations at once which makes it less intrusive. But at the end of the day, it is necessary to annotate the declarations that were added to the dylib, and anything that uses those transitively.

Also, a lot of these symbols are inline and don't depend on anything in the dylib. Why are the being marked unavailable?

This patch did annotate too many things -- I'll submit a follow-up that does not annotate as many things. For example the perms enumeration and its associated operations don't need to be annotated. However, one must keep in mind that I need to annotate anything that uses a symbol in the dylib, and do that transitively. So some things that might not seem to be implemented in the dylib still need to be annotated because they are using something that's in the dylib (sometimes indirectly).

ldionne updated this revision to Diff 190115.Mar 11 2019, 9:49 AM

Remove markup on inline enums, and add markup for path.

I had somehow forgotten path in the previous patch.. oops.

With the new patch (which removes markup from unnecessary methods and classes), I think this is actually the right approach. Using an attribute wouldn't buy us very much as we'd have to close the namespace, push the pragma, open the namespace and then close it again to pop the pragma. And the only set of declarations this would cleanly apply to is the set of operations on path. So I think this is as simple as we can get now.

Now, I understand folks don't like availability markup (and markup in general). I share that distaste because it clutters the source code. So I would encourage us to work on a better general solution if we can find one:

  • maybe the markup can somehow be stored externally to the library?
  • maybe we can automatically derive the list of declarations in the compiler by knowing what symbols were added and when?
  • maybe it's easy to write an external tool to check this instead of having it in the library?

However, we shouldn't stop progress on putting <filesystem> into the dylib while we're waiting for a better general solution to this problem. Once/if we have such a solution, we can come back and apply it throughout the library (<variant>, <optional> & friends would also benefit).

ldionne abandoned this revision.Mar 15 2019, 7:46 AM

I'm dropping this in favour of https://reviews.llvm.org/D59224, which is less intrusive.