Granularize the <filesystem> header
Details
- Reviewers
• Quuxplusone ldionne Mordante - Group Reviewers
Restricted Project - Commits
- rG7056250f517a: [libc++][NFC] Granularize <filesystem>
Diff Detail
- Repository
- rG LLVM Github Monorepo
Unit Tests
Event Timeline
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. |
libcxx/include/filesystem | ||
314–315 | 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? |
- Moved ranges to correct files
- Removed __system_complete
- ran ninja libcxx-generate-files
libcxx/include/filesystem | ||
---|---|---|
314–315 | Sorry, I forgot to comment here. I just didn't know if I should remove it in this PR. |
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 | ||
---|---|---|
41 | Since this spans the whole file, we should have a closing comment here. | |
43 | All these headers are missing a closing // _LIBCPP___FILESYSTEM_ABSOLUTE_H comment. | |
libcxx/include/filesystem | ||
314–315 | 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. |
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). |
libcxx/include/filesystem | ||
---|---|---|
306–307 | 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.... 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". |
This LGTM now, and we can try to clean up the unnecessary includes in <filesystem> in a separate patch.
libcxx/include/filesystem | ||
---|---|---|
306–307 | 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. |
Since this spans the whole file, we should have a closing comment here.