<div dir="ltr"><div>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.</div><div><br></div><div>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.</div><div><br></div><div>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.</div><div><br></div><div>/Eric</div><div><br></div><div class="gmail_quote"><div dir="ltr">On Tue, Aug 7, 2018 at 2:15 PM Louis Dionne <<a href="mailto:ldionne@apple.com" target="_blank">ldionne@apple.com</a>> wrote:<br></div><blockquote class="gmail_quote" style="margin:0 0 0 .8ex;border-left:1px #ccc solid;padding-left:1ex">Hi,<br>
<br>
My current understanding of the problem (based on <a href="https://reviews.llvm.org/D49774" rel="noreferrer" target="_blank">https://reviews.llvm.org/D49774</a>) 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.<br>
<br>
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?<br>
<br>
If that is correct, then we can either<br>
(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<br>
(2) do nothing and live with an ABI break in C++20<br>
(3) revert r338093 (and perhaps some patches that depend on it) in the LLVM 7 branch, and keep shipping Filesystem as being experimental<br>
<br>
(1) is ideal, but we have to confirm that my understanding is correct AND agree on a fix, soon.<br>
(2) is a no-go.<br>
(3) is the safest and simplest option.<br>
<br>
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.<br>
<br>
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.<br>
<br>
Louis<br>
<br>
<br>
> On Aug 2, 2018, at 03:46, Hans Wennborg <<a href="mailto:hans@chromium.org" target="_blank">hans@chromium.org</a>> wrote:<br>
> <br>
> I only saw this now after the llvm 7 branch was cut.<br>
> <br>
> It would be good to get this all figured out before the release :-)<br>
> <br>
> On Mon, Jul 30, 2018 at 10:19 PM, Louis Dionne via llvm-dev<br>
> <<a href="mailto:llvm-dev@lists.llvm.org" target="_blank">llvm-dev@lists.llvm.org</a>> wrote:<br>
>> FWIW, I’d like for us to come to an agreement before the branch for LLVM 7.0<br>
>> is cut. How do others feel about this? Am I wrong when I claim that shipping<br>
>> an ABI-unstable feature in the std:: namespace is a deviation from normal<br>
>> practice? Am I overcautious when I say it’s asking for trouble?<br>
>> <br>
>> Eric, I know you’re busy and may not have time to do the work so I’m totally<br>
>> willing to chime in, but I’d like to have your thoughts on my objection<br>
>> first.<br>
>> <br>
>> Cheers,<br>
>> Louis<br>
>> <br>
>> <br>
>> On Jul 27, 2018, at 17:02, Louis Dionne via cfe-dev <<a href="mailto:cfe-dev@lists.llvm.org" target="_blank">cfe-dev@lists.llvm.org</a>><br>
>> wrote:<br>
>> <br>
>> Eric,<br>
>> <br>
>> I’m curious to know what the concerns are w.r.t. providing ABI stability for<br>
>> filesystem right now. What do you envision may require changing the ABI in<br>
>> the future?<br>
>> <br>
>> I feel like taking filesystem out of experimental/ without providing the<br>
>> usual guarantees provided by libc++ for non-experimental code may not be a<br>
>> good idea, as we’ll be pretending that we support filesystem when we really<br>
>> only half support it. In other words, I think the number of people that will<br>
>> start using filesystem while _consciously_ knowing that it is ABI-unstable<br>
>> (and what that means) is quite small, and that is making our users a<br>
>> disservice.<br>
>> <br>
>> Would it be possible to instead ship the parts we’re not quite sure we can<br>
>> keep ABI stable in the headers (with `_LIBCPP_HIDE_FROM_ABI`) for the time<br>
>> being, and lower them to the dylib eventually as things stabilize? This<br>
>> would allow us to ship filesystem with LLVM 7.0 without any compromise on<br>
>> the guarantees we make our users.<br>
>> <br>
>> I’m curious to know what you think of this suggestion.<br>
>> Louis<br>
>> <br>
>> On Jul 27, 2018, at 00:20, Eric Fiselier via cfe-dev<br>
>> <<a href="mailto:cfe-dev@lists.llvm.org" target="_blank">cfe-dev@lists.llvm.org</a>> wrote:<br>
>> <br>
>> Hi All,<br>
>> <br>
>> I recently committed <filesystem> to trunk. I wanted to bring attention to<br>
>> some quirks it currently has.<br>
>> <br>
>> First, it's been put in a separate library, libc++fs, for now. Users are<br>
>> responsible for linking the library when they use filesystem.<br>
>> <br>
>> Second, it should still not be considered ABI stable. Vendors should be<br>
>> aware of this before shipping it. Hopefully all the standard and<br>
>> implementation bugs can be resolved by the next release, and we can move it<br>
>> into the main dylib.<br>
>> <br>
>> Third, libc++experimental no longer contains the symbols for<br>
>> <experimental/filesystem>, which is really just <filesystem> is disguise. If<br>
>> you've been using <experimental/filesystem> you now need to link libc++fs<br>
>> instead.<br>
>> <br>
>> Fourth, `<filesystem>` is technically available in C++11 and later. The<br>
>> implementation lives in the std::__fs::filesystem namespace, which is marked<br>
>> "inline" in C++17 but not before. We should consider documenting this as an<br>
>> extension to its use w/o C++17.<br>
>> <br>
>> Happy coding,<br>
>> <br>
>> /Eric<br>
>> <br>
>> [1]<br>
>> <a href="http://libcxx.llvm.org/docs/UsingLibcxx.html#using-filesystem-and-libc-fs" rel="noreferrer" target="_blank">http://libcxx.llvm.org/docs/UsingLibcxx.html#using-filesystem-and-libc-fs</a><br>
>> _______________________________________________<br>
>> cfe-dev mailing list<br>
>> <a href="mailto:cfe-dev@lists.llvm.org" target="_blank">cfe-dev@lists.llvm.org</a><br>
>> <a href="http://lists.llvm.org/cgi-bin/mailman/listinfo/cfe-dev" rel="noreferrer" target="_blank">http://lists.llvm.org/cgi-bin/mailman/listinfo/cfe-dev</a><br>
>> <br>
>> <br>
>> _______________________________________________<br>
>> cfe-dev mailing list<br>
>> <a href="mailto:cfe-dev@lists.llvm.org" target="_blank">cfe-dev@lists.llvm.org</a><br>
>> <a href="http://lists.llvm.org/cgi-bin/mailman/listinfo/cfe-dev" rel="noreferrer" target="_blank">http://lists.llvm.org/cgi-bin/mailman/listinfo/cfe-dev</a><br>
>> <br>
>> <br>
>> <br>
>> _______________________________________________<br>
>> LLVM Developers mailing list<br>
>> <a href="mailto:llvm-dev@lists.llvm.org" target="_blank">llvm-dev@lists.llvm.org</a><br>
>> <a href="http://lists.llvm.org/cgi-bin/mailman/listinfo/llvm-dev" rel="noreferrer" target="_blank">http://lists.llvm.org/cgi-bin/mailman/listinfo/llvm-dev</a><br>
>> <br>
<br>
</blockquote></div></div>