llvm dev - Aug 2018 - [cfe-dev] Filesystem has Landed in Libc++

Why did you want the symbols moved out of libc++experimental, and for the
header to be moved from <experimental/filesystem> to <filesystem>?
It
certainly seems like it'd be safer and clearer to move them back to the old
locations, but it's not clear to me if that'd be trading off something
else
of value.

Was there some other greater purpose served by the change in location,
which will be hindered by moving them back where they were until
ABI-stability can be promised?




On Thu, Aug 9, 2018 at 1:54 PM Eric Fiselier via llvm-dev <
llvm-dev at lists.llvm.org> wrote:
> This isn't about C++20 and the <chrono> changes. I have no doubt
any ABI
> breaking changes W.R.T. that will be addressed.
>
> During my work finishing up filesystem, I discovered/filed a number of
> issues with ABI breaking concerns, in particular around directory_entry,
> but perhaps other places as well (recursive_directory_iterator, various
> calls in [fs.op.funcs]). These issues have not yet been addressed in the
> standard, and these spec should be addressed before we commit to an ABI.
>
> Note that we ofter put things into namespace std before they're
considered
> ABI stable, but up until now that has been limited to components specified
> by a standard still in flight. Also note that libstdc++ also hasn't
> committed to ABI stability for <filesystem> yet. This is the reason
we put
> the symbols in a separate static library.
>
> /Eric
>
> On Tue, Aug 7, 2018 at 2:15 PM Louis Dionne <ldionne at apple.com>
wrote:
>
>> Hi,
>>
>> My current understanding of the problem (based on
>> https://reviews.llvm.org/D49774) is that we have a type,
file_time_type,
>> which is part of the ABI and is currently defined as
>> std::chrono::time_point<_FileSystemClock>, where _FileSystemClock
is an
>> internal type represented using a __int128_t. However, C++20 will add a
>> type called file_clock and redefine file_time_type to be
>> std::chrono::time_point<std::chrono::file_clock> instead, which
is an ABI
>> break.
>>
>> Is this correct, and is this the only concern we have with respect to
the
>> ABI stability of Filesystem as currently included in the release branch
for
>> LLVM 7?
>>
>> If that is correct, then we can either
>> (1) define file_time_type in a way that is forward-ABI-compatible with
>> the definition that will be used in C++20 and patch LLVM 7 accordingly
>> (2) do nothing and live with an ABI break in C++20
>> (3) revert r338093 (and perhaps some patches that depend on it) in the
>> LLVM 7 branch, and keep shipping Filesystem as being experimental
>>
>> (1) is ideal, but we have to confirm that my understanding is correct
AND
>> agree on a fix, soon.
>> (2) is a no-go.
>> (3) is the safest and simplest option.
>>
>> If we can't get a solution for (1) going very soon, I would like to
>> request that we implement (3) and remove Filesystem from the
>> non-experimental namespace in the LLVM 7 branch. This is unfortunate,
but
>> we're currently running towards (2), which is desirable for nobody.
>>
>> I suggest a deadline of Friday August 10th to have settled on a
solution
>> for (1) so as to give us time to implement it and review it in time for
RC
>> 2, on August 22nd.
>>
>> Louis
>>
>>
>> > On Aug 2, 2018, at 03:46, Hans Wennborg <hans at
chromium.org> wrote:
>> >
>> > I only saw this now after the llvm 7 branch was cut.
>> >
>> > It would be good to get this all figured out before the release
:-)
>> >
>> > On Mon, Jul 30, 2018 at 10:19 PM, Louis Dionne via llvm-dev
>> > <llvm-dev at lists.llvm.org> wrote:
>> >> FWIW, I’d like for us to come to an agreement before the
branch for
>> LLVM 7.0
>> >> is cut. How do others feel about this? Am I wrong when I claim
that
>> shipping
>> >> an ABI-unstable feature in the std:: namespace is a deviation
from
>> normal
>> >> practice? Am I overcautious when I say it’s asking for
trouble?
>> >>
>> >> Eric, I know you’re busy and may not have time to do the work
so I’m
>> totally
>> >> willing to chime in, but I’d like to have your thoughts on my
objection
>> >> first.
>> >>
>> >> Cheers,
>> >> Louis
>> >>
>> >>
>> >> On Jul 27, 2018, at 17:02, Louis Dionne via cfe-dev <
>> cfe-dev at lists.llvm.org>
>> >> wrote:
>> >>
>> >> Eric,
>> >>
>> >> I’m curious to know what the concerns are w.r.t. providing ABI
>> stability for
>> >> filesystem right now. What do you envision may require
changing the
>> ABI in
>> >> the future?
>> >>
>> >> I feel like taking filesystem out of experimental/ without
providing
>> the
>> >> usual guarantees provided by libc++ for non-experimental code
may not
>> be a
>> >> good idea, as we’ll be pretending that we support filesystem
when we
>> really
>> >> only half support it. In other words, I think the number of
people
>> that will
>> >> start using filesystem while _consciously_ knowing that it is
>> ABI-unstable
>> >> (and what that means) is quite small, and that is making our
users a
>> >> disservice.
>> >>
>> >> Would it be possible to instead ship the parts we’re not quite
sure we
>> can
>> >> keep ABI stable in the headers (with `_LIBCPP_HIDE_FROM_ABI`)
for the
>> time
>> >> being, and lower them to the dylib eventually as things
stabilize? This
>> >> would allow us to ship filesystem with LLVM 7.0 without any
compromise
>> on
>> >> the guarantees we make our users.
>> >>
>> >> I’m curious to know what you think of this suggestion.
>> >> Louis
>> >>
>> >> On Jul 27, 2018, at 00:20, Eric Fiselier via cfe-dev
>> >> <cfe-dev at lists.llvm.org> wrote:
>> >>
>> >> Hi All,
>> >>
>> >> I recently committed <filesystem> to trunk. I wanted to
bring
>> attention to
>> >> some quirks it currently has.
>> >>
>> >> First, it's been put in a separate library, libc++fs, for
now. Users
>> are
>> >> responsible for linking the library when they use filesystem.
>> >>
>> >> Second, it should still not be considered ABI stable. Vendors
should be
>> >> aware of this before shipping it. Hopefully all the standard
and
>> >> implementation bugs can be resolved by the next release, and
we can
>> move it
>> >> into the main dylib.
>> >>
>> >> Third, libc++experimental no longer contains the symbols for
>> >> <experimental/filesystem>, which is really just
<filesystem> is
>> disguise. If
>> >> you've been using <experimental/filesystem> you now
need to link
>> libc++fs
>> >> instead.
>> >>
>> >> Fourth, `<filesystem>` is technically available in C++11
and later. The
>> >> implementation lives in the std::__fs::filesystem namespace,
which is
>> marked
>> >> "inline" in C++17 but not before. We should consider
documenting this
>> as an
>> >> extension to its use w/o C++17.
>> >>
>> >> Happy coding,
>> >>
>> >> /Eric
>> >>
>> >> [1]
>> >>
>>
http://libcxx.llvm.org/docs/UsingLibcxx.html#using-filesystem-and-libc-fs
>> >> _______________________________________________
>> >> cfe-dev mailing list
>> >> cfe-dev at lists.llvm.org
>> >> http://lists.llvm.org/cgi-bin/mailman/listinfo/cfe-dev
>> >>
>> >>
>> >> _______________________________________________
>> >> cfe-dev mailing list
>> >> cfe-dev at lists.llvm.org
>> >> http://lists.llvm.org/cgi-bin/mailman/listinfo/cfe-dev
>> >>
>> >>
>> >>
>> >> _______________________________________________
>> >> LLVM Developers mailing list
>> >> llvm-dev at lists.llvm.org
>> >> http://lists.llvm.org/cgi-bin/mailman/listinfo/llvm-dev
>> >>
>>
>> _______________________________________________
> LLVM Developers mailing list
> llvm-dev at lists.llvm.org
> http://lists.llvm.org/cgi-bin/mailman/listinfo/llvm-dev
>
-------------- next part --------------
An HTML attachment was scrubbed...
URL:
<http://lists.llvm.org/pipermail/llvm-dev/attachments/20180809/77ab4ac7/attachment.html>

Well, technically <experimental/filesystem> tracks one specification,
<filesystem> another. I normally advocate for libc++'s
<experimental/foo>
to act like the specification for <foo>, but not all STLs do that.

Therefore, the purpose of the change was to allow users to write portable
code across multiple STL's using <filesystem> (minus the changes to
the
build system needed to link it).

/Eric

On Thu, Aug 9, 2018 at 2:10 PM James Y Knight <jyknight at google.com>
wrote:
> Why did you want the symbols moved out of libc++experimental, and for the
> header to be moved from <experimental/filesystem> to
<filesystem>? It
> certainly seems like it'd be safer and clearer to move them back to the
old
> locations, but it's not clear to me if that'd be trading off
something else
> of value.
>
> Was there some other greater purpose served by the change in location,
> which will be hindered by moving them back where they were until
> ABI-stability can be promised?
>
>
>
>
> On Thu, Aug 9, 2018 at 1:54 PM Eric Fiselier via llvm-dev <
> llvm-dev at lists.llvm.org> wrote:
>
>> This isn't about C++20 and the <chrono> changes. I have no
doubt any ABI
>> breaking changes W.R.T. that will be addressed.
>>
>> During my work finishing up filesystem, I discovered/filed a number of
>> issues with ABI breaking concerns, in particular around
directory_entry,
>> but perhaps other places as well (recursive_directory_iterator, various
>> calls in [fs.op.funcs]). These issues have not yet been addressed in
the
>> standard, and these spec should be addressed before we commit to an
ABI.
>>
>> Note that we ofter put things into namespace std before they're
>> considered ABI stable, but up until now that has been limited to
components
>> specified by a standard still in flight. Also note that libstdc++ also
>> hasn't committed to ABI stability for <filesystem> yet. This
is the reason
>> we put the symbols in a separate static library.
>>
>> /Eric
>>
>> On Tue, Aug 7, 2018 at 2:15 PM Louis Dionne <ldionne at
apple.com> wrote:
>>
>>> Hi,
>>>
>>> My current understanding of the problem (based on
>>> https://reviews.llvm.org/D49774) is that we have a type,
>>> file_time_type, which is part of the ABI and is currently defined
as
>>> std::chrono::time_point<_FileSystemClock>, where
_FileSystemClock is an
>>> internal type represented using a __int128_t. However, C++20 will
add a
>>> type called file_clock and redefine file_time_type to be
>>> std::chrono::time_point<std::chrono::file_clock> instead,
which is an ABI
>>> break.
>>>
>>> Is this correct, and is this the only concern we have with respect
to
>>> the ABI stability of Filesystem as currently included in the
release branch
>>> for LLVM 7?
>>>
>>> If that is correct, then we can either
>>> (1) define file_time_type in a way that is forward-ABI-compatible
with
>>> the definition that will be used in C++20 and patch LLVM 7
accordingly
>>> (2) do nothing and live with an ABI break in C++20
>>> (3) revert r338093 (and perhaps some patches that depend on it) in
the
>>> LLVM 7 branch, and keep shipping Filesystem as being experimental
>>>
>>> (1) is ideal, but we have to confirm that my understanding is
correct
>>> AND agree on a fix, soon.
>>> (2) is a no-go.
>>> (3) is the safest and simplest option.
>>>
>>> If we can't get a solution for (1) going very soon, I would
like to
>>> request that we implement (3) and remove Filesystem from the
>>> non-experimental namespace in the LLVM 7 branch. This is
unfortunate, but
>>> we're currently running towards (2), which is desirable for
nobody.
>>>
>>> I suggest a deadline of Friday August 10th to have settled on a
solution
>>> for (1) so as to give us time to implement it and review it in time
for RC
>>> 2, on August 22nd.
>>>
>>> Louis
>>>
>>>
>>> > On Aug 2, 2018, at 03:46, Hans Wennborg <hans at
chromium.org> wrote:
>>> >
>>> > I only saw this now after the llvm 7 branch was cut.
>>> >
>>> > It would be good to get this all figured out before the
release :-)
>>> >
>>> > On Mon, Jul 30, 2018 at 10:19 PM, Louis Dionne via llvm-dev
>>> > <llvm-dev at lists.llvm.org> wrote:
>>> >> FWIW, I’d like for us to come to an agreement before the
branch for
>>> LLVM 7.0
>>> >> is cut. How do others feel about this? Am I wrong when I
claim that
>>> shipping
>>> >> an ABI-unstable feature in the std:: namespace is a
deviation from
>>> normal
>>> >> practice? Am I overcautious when I say it’s asking for
trouble?
>>> >>
>>> >> Eric, I know you’re busy and may not have time to do the
work so I’m
>>> totally
>>> >> willing to chime in, but I’d like to have your thoughts on
my
>>> objection
>>> >> first.
>>> >>
>>> >> Cheers,
>>> >> Louis
>>> >>
>>> >>
>>> >> On Jul 27, 2018, at 17:02, Louis Dionne via cfe-dev <
>>> cfe-dev at lists.llvm.org>
>>> >> wrote:
>>> >>
>>> >> Eric,
>>> >>
>>> >> I’m curious to know what the concerns are w.r.t. providing
ABI
>>> stability for
>>> >> filesystem right now. What do you envision may require
changing the
>>> ABI in
>>> >> the future?
>>> >>
>>> >> I feel like taking filesystem out of experimental/ without
providing
>>> the
>>> >> usual guarantees provided by libc++ for non-experimental
code may not
>>> be a
>>> >> good idea, as we’ll be pretending that we support
filesystem when we
>>> really
>>> >> only half support it. In other words, I think the number
of people
>>> that will
>>> >> start using filesystem while _consciously_ knowing that it
is
>>> ABI-unstable
>>> >> (and what that means) is quite small, and that is making
our users a
>>> >> disservice.
>>> >>
>>> >> Would it be possible to instead ship the parts we’re not
quite sure
>>> we can
>>> >> keep ABI stable in the headers (with
`_LIBCPP_HIDE_FROM_ABI`) for the
>>> time
>>> >> being, and lower them to the dylib eventually as things
stabilize?
>>> This
>>> >> would allow us to ship filesystem with LLVM 7.0 without
any
>>> compromise on
>>> >> the guarantees we make our users.
>>> >>
>>> >> I’m curious to know what you think of this suggestion.
>>> >> Louis
>>> >>
>>> >> On Jul 27, 2018, at 00:20, Eric Fiselier via cfe-dev
>>> >> <cfe-dev at lists.llvm.org> wrote:
>>> >>
>>> >> Hi All,
>>> >>
>>> >> I recently committed <filesystem> to trunk. I wanted
to bring
>>> attention to
>>> >> some quirks it currently has.
>>> >>
>>> >> First, it's been put in a separate library, libc++fs,
for now. Users
>>> are
>>> >> responsible for linking the library when they use
filesystem.
>>> >>
>>> >> Second, it should still not be considered ABI stable.
Vendors should
>>> be
>>> >> aware of this before shipping it. Hopefully all the
standard and
>>> >> implementation bugs can be resolved by the next release,
and we can
>>> move it
>>> >> into the main dylib.
>>> >>
>>> >> Third, libc++experimental no longer contains the symbols
for
>>> >> <experimental/filesystem>, which is really just
<filesystem> is
>>> disguise. If
>>> >> you've been using <experimental/filesystem> you
now need to link
>>> libc++fs
>>> >> instead.
>>> >>
>>> >> Fourth, `<filesystem>` is technically available in
C++11 and later.
>>> The
>>> >> implementation lives in the std::__fs::filesystem
namespace, which is
>>> marked
>>> >> "inline" in C++17 but not before. We should
consider documenting this
>>> as an
>>> >> extension to its use w/o C++17.
>>> >>
>>> >> Happy coding,
>>> >>
>>> >> /Eric
>>> >>
>>> >> [1]
>>> >>
>>>
http://libcxx.llvm.org/docs/UsingLibcxx.html#using-filesystem-and-libc-fs
>>> >> _______________________________________________
>>> >> cfe-dev mailing list
>>> >> cfe-dev at lists.llvm.org
>>> >> http://lists.llvm.org/cgi-bin/mailman/listinfo/cfe-dev
>>> >>
>>> >>
>>> >> _______________________________________________
>>> >> cfe-dev mailing list
>>> >> cfe-dev at lists.llvm.org
>>> >> http://lists.llvm.org/cgi-bin/mailman/listinfo/cfe-dev
>>> >>
>>> >>
>>> >>
>>> >> _______________________________________________
>>> >> LLVM Developers mailing list
>>> >> llvm-dev at lists.llvm.org
>>> >> http://lists.llvm.org/cgi-bin/mailman/listinfo/llvm-dev
>>> >>
>>>
>>> _______________________________________________
>> LLVM Developers mailing list
>> llvm-dev at lists.llvm.org
>> http://lists.llvm.org/cgi-bin/mailman/listinfo/llvm-dev
>>
>
-------------- next part --------------
An HTML attachment was scrubbed...
URL:
<http://lists.llvm.org/pipermail/llvm-dev/attachments/20180809/d191ff89/attachment.html>

On Thu, Aug 9, 2018 at 8:50 PM, Eric Fiselier <eric at efcs.ca> wrote:
> Well, technically <experimental/filesystem> tracks one specification,
> <filesystem> another. I normally advocate for libc++'s
<experimental/foo>
> to act like the specification for <foo>, but not all STLs do that.
>
Right. I'm not a fan of that approach.
     <experimental/optional> matched what was in the library fundamental
TS.
     <optional> matched what was in the IS.

-- Marshall
-------------- next part --------------
An HTML attachment was scrubbed...
URL:
<http://lists.llvm.org/pipermail/llvm-dev/attachments/20180810/39bece69/attachment.html>