This is an archive of the discontinued LLVM Phabricator instance.

[libc++][NFC] Granularize <filesystem>
ClosedPublic

Authored by philnik on Dec 11 2021, 7:57 AM.

Details

Summary

Granularize the <filesystem> header

Diff Detail

Event Timeline

philnik created this revision.Dec 11 2021, 7:57 AM
philnik requested review of this revision.Dec 11 2021, 7:57 AM
Herald added a project: Restricted Project. · View Herald TranscriptDec 11 2021, 7:57 AM
Herald added a reviewer: Restricted Project. · View Herald Transcript
Quuxplusone requested changes to this revision.Dec 11 2021, 6:54 PM

You need to run ninja libcxx-generate-files (or python ../libcxx/utils/generate[whatever].py directly) for the buildkite failure.
One substantive file-organization comment, one "remember to clean this up" comment; otherwise LGTM.

libcxx/include/__filesystem/ranges.h
21–33 ↗(On Diff #393679)

These specializations belong in directory_iterator.h and recursive_directory_iterator.h, respectively, and this file should disappear.
Think of std specializations (std::hash, std::is_error_code, std::enable_borrowed_range) the same way you'd think of ADL functions (swap, operator==) or deduction guides: part of the type's API.

libcxx/include/filesystem
276–277

I guess this single declaration remains here because you think it's dead code (hence not worth moving into a permanent new home) but you're being really careful not to change any behavior in this NFC patch?
As long as one of us makes a followup commit to remove this declaration, I'm OK with this approach. But I would enthusiastically +1 simply removing this declaration in this same commit, too.

This revision now requires changes to proceed.Dec 11 2021, 6:54 PM
philnik updated this revision to Diff 393735.Dec 12 2021, 2:19 AM
philnik marked an inline comment as done.
  • Moved ranges to correct files
  • Removed __system_complete
  • ran ninja libcxx-generate-files
philnik added inline comments.Dec 12 2021, 2:20 AM
libcxx/include/filesystem
276–277

Sorry, I forgot to comment here. I just didn't know if I should remove it in this PR.

philnik updated this revision to Diff 393739.Dec 12 2021, 3:55 AM
  • Removed ranges.h references
philnik updated this revision to Diff 393743.Dec 12 2021, 5:06 AM
  • Added version guards
philnik updated this revision to Diff 393766.Dec 12 2021, 10:56 AM

Removed <__locale> include

ldionne requested changes to this revision.Dec 13 2021, 6:07 AM

Given that most filesystem operations simply make a call to the dylib and they all share almost the same dependencies, it seems overkill to me to split them in separate files. Instead, I'd rather have one header that contains all the operations, something like __filesystem/operations.h. WDYT?

This revision now requires changes to proceed.Dec 13 2021, 6:07 AM

Given that most filesystem operations simply make a call to the dylib and they all share almost the same dependencies, it seems overkill to me to split them in separate files. Instead, I'd rather have one header that contains all the operations, something like __filesystem/operations.h. WDYT?

FWIW, I wouldn't object to that. But (1) it's less clearly defined, compared to the "one file per overload set" rule; and (2) some of these overload sets do have 2, 3, or 4 members, so at least it's not like the granular files are laughably trivial. :)
Yet another option would be to pack all the dylib functions' prototypes together in e.g. <__filesystem/fwd.h> (I can't think of a good name), and then including fwd.h from all the granular headers. However, sprinkling them one-by-one throughout the files is essentially the same thing we do for the underscore versions of algorithms in <algorithm> — e.g. we put __push_heap in the same header with push_heap, albeit for good perf reasons in that case — so there's a sort of precedent for declaring e.g. __weakly_canonical in the same header with weakly_canonical.
In any case, my understanding is that the failure mode we're trying to protect against is that we accidentally forget one of the dylib functions, or we slip in an extra prototype that doesn't exist in the dylib, or something like that. (In fact, that seems to have already happened, with __system_complete.)

In short: *shrug*.

My only concern is readability. I feel that stuffing all these one-liners in a single file provides the most readability and the least duplication. When there is a repetitive structure that can be highlighted in the code, it's usually a good idea to highlight it. By putting all the operations in the same header, we'd be able to nicely highlight that they are all _LIBCPP_HIDE_FROM_ABI helpers calling into the dylib, and we could even format them differently (in a NFC patch) to highlight additional similarities between them. The algorithms in <algorithm> are different cause they don't share much similarity and they have non-trivial implementations.

libcxx/include/__filesystem/absolute.h
40 ↗(On Diff #393766)

Since this spans the whole file, we should have a closing comment here.

42 ↗(On Diff #393766)

All these headers are missing a closing // _LIBCPP___FILESYSTEM_ABSOLUTE_H comment.

libcxx/include/filesystem
276–277

Not removing it in this PR was a great and cautious move. Normally we don't make any functional changes in these patches. Sometimes, it could go as far as a declaration existing and being defined in a vendor's downstream fork only, and removing the declaration could cause havoc. If that were the case, I'd argue the vendor messed up in the first place, but my point is that things are often subtler than they seem.

In this case, I think that's a simple oversight and removing it in this PR is OK since it's been called out. But in general, you had the right approach the first time around.

General comment: being consistent and diligent about splitting NFCs from non-NFCs and making orthogonal commits is hugely helpful when things do go wrong. For example, it allows bisecting automatically, and it makes it so much easier for whoever's trying to understand the failure. Making multiple changes at a time is really painful especially for people who get those bug reports, which is often vendors since they are the interface layer between libc++ developers and libc++ users.

philnik updated this revision to Diff 394854.Dec 16 2021, 6:46 AM
philnik marked 4 inline comments as done.
  • Put all operations in operations.h
philnik updated this revision to Diff 394856.Dec 16 2021, 6:47 AM
  • ran ninja libcxx-generate-files
philnik updated this revision to Diff 394863.Dec 16 2021, 7:08 AM
  • Updated includes in directory_entry.h
ldionne accepted this revision.Dec 16 2021, 9:12 AM

This LGTM but we have to figure out why CI is failing. Thanks for modularizing this!

libcxx/include/__filesystem/file_type.h
21

I think the Apple back-deployment CI is failing because this _LIBCPP_AVAILABILITY_FILESYSTEM_PUSH/_LIBCPP_AVAILABILITY_FILESYSTEM_POP` pair is a no-op (it doesn't apply the attribute to any declaration in-between, because there's only an enum in-between). Removing this pair should fix the CI, and I think that's correct because we don't want to mark that enum as unavailable (well I guess we don't care whether it is marked or not, since it has no dependency on the dylib content).

philnik updated this revision to Diff 394985.Dec 16 2021, 1:43 PM
  • Removed _LIBCPP_AVAILABILITY_* in file_type.h
philnik marked an inline comment as done.Dec 16 2021, 1:43 PM
philnik updated this revision to Diff 395080.Dec 17 2021, 2:36 AM
  • Removed _LIBCPP_AVAILABILITY* in file_time_type.h
  • Added headers
philnik updated this revision to Diff 395158.Dec 17 2021, 10:07 AM
  • Changed includes
philnik updated this revision to Diff 395183.Dec 17 2021, 11:38 AM
  • Added <system_error> include to <filesystem>
philnik updated this revision to Diff 395200.Dec 17 2021, 12:33 PM
  • Added <string> include in <__filesystem/oath_iterator.h>
libcxx/include/filesystem
268–269

I see your repeated updates fiddling with #includes here, and I imagine they're to fix test failures with Modules? I believe that you should actually leave all of the public headers #included here, i.e. please re-add <chrono> and <cstddef> and <cstdlib> and <iosfwd> and....
(And re-alphabetize; you've got <system_error> below <version> right now.)

That will make this refactoring truly NFC.

Then, feel free to follow up with a PR to remove all these now-unused headers... but knowing that that will break anyone who's currently using <filesystem> with Modules and accidentally depending on these transitive includes, so it's not "NFC", it's just a good idea. In that same PR, you would also update any libcxx/test/ files that are currently relying on these transitive includes to "Include What You Use".

philnik updated this revision to Diff 395270.Dec 18 2021, 1:22 AM
  • Re-added all includes back in <filesystem>
philnik marked an inline comment as done.Dec 18 2021, 1:23 AM
ldionne accepted this revision.Dec 20 2021, 8:15 AM

This LGTM now, and we can try to clean up the unnecessary includes in <filesystem> in a separate patch.

libcxx/include/filesystem
268–269

Just to give additional background on this: people unfortunately often have missing includes in their code. As a result, they often rely on one of our transitive includes without realizing it, and we break a lot of code when we "clean up" our includes like that.

We're in favour of doing such clean up, however we need to be careful not to mix these changes with seemingly innocuous changes, otherwise things become much harder when shipping the library.

Quuxplusone accepted this revision.Dec 21 2021, 5:12 PM
This revision is now accepted and ready to land.Dec 21 2021, 5:12 PM
This revision was automatically updated to reflect the committed changes.