This is an archive of the discontinued LLVM Phabricator instance.

[libc++] Alphabetize headers, and enforce the order in buildkite. NFCI.
ClosedPublic

Authored by Quuxplusone on Jan 7 2022, 6:50 AM.

Details

Summary

The really relevant change is in "graph_header_deps.py"; everything else is just rearranging headers.
In a few places, the out-of-order header wasn't being used, so I just removed it.

As long as locales don't do anything weird, this should finally put to rest the question "Does . sort before or after _?" in a way we can at least be consistent about.

Diff Detail

Event Timeline

Quuxplusone created this revision.Jan 7 2022, 6:50 AM
Quuxplusone requested review of this revision.Jan 7 2022, 6:50 AM
Herald added 1 blocking reviewer(s): Restricted Project. · View Herald Transcript

Running clang-tidy double_include.sh.cpp -header-filter=.* -system-headers -- -nostdinc++ -I ../../../build/include/c++/v1/ -std=c++2b it complains about

  • the ranges namespaces (which I find reasonable)
  • non-inline variables in reverse_view.h (they are constexpr, so this seems to be a clang-tidy bug)
  • a bunch of include-order problems in non-libc++ system headers (I don't know how to only get the results for our headers)
  • 1 include-order warning in __filesystem/path.h (It doesn't like the <locale> and <iomanip> order)
  • misc-redundant-expression in __iterator/iterator_traits.h:201 (this looks very suspicious, but I don't know if that is a bug)

There are about 30 libc++-related warnings total, most of which are the ranges namespace comments. So I think it is, with the clang-tidy bug fixed, quite reasonable to enable clang-tidy for our include/-files.
I haven't checked src/ fully, but just going through a few headers, it seems to almost exclusively be include order and namespace comments

Could you also reorder the headers in modules.modulemap and CMakeLists.txt?

I don't think we're passing --check-libcxx-invariants by default in the CI right now, so the new code in graph_header_deps.py isn't being exercised unless I missed something.

libcxx/include/__algorithm/copy_n.h
16

This collateral removal is safe because that header is included in copy.h anyways.

libcxx/include/__algorithm/partial_sort_copy.h
19

I'll assume this is being included by other headers in <algorithm>, and hence is safe to remove.

libcxx/include/__algorithm/partition.h
15

I'll assume this is being included by other headers in <algorithm>, and hence is safe to remove.

Mordante accepted this revision as: Mordante.Jan 10 2022, 8:57 AM

Nice cleanup!

I don't think we're passing --check-libcxx-invariants by default in the CI right now, so the new code in graph_header_deps.py isn't being exercised unless I missed something.

I also would like to see it enabled.
I didn't see oddities and the modular build passes, LGTM!

libcxx/include/__algorithm/partial_sort_copy.h
19

I don't see a call to swap in this header.

ldionne added inline comments.Jan 10 2022, 9:35 AM
libcxx/include/__algorithm/partial_sort_copy.h
19

(my comment was referring to the fact that removing transitive includes can break users that don't IWYU properly)

Quuxplusone marked an inline comment as done.Jan 10 2022, 9:50 AM
Quuxplusone added inline comments.
libcxx/include/__algorithm/partition.h
15

Yep, <algorithm> itself currently contains

#include <cstddef>
#include <cstring>
#include <functional>
#include <initializer_list>
#include <iterator>
#include <memory>
#include <type_traits>
#include <utility> // swap_ranges
#include <version>

which I think is the correct strategy for Hyrum's Law/IWYU issues.

Actually enforce the invariant in the buildkite script!

ldionne accepted this revision.Jan 10 2022, 12:32 PM
This revision is now accepted and ready to land.Jan 10 2022, 12:32 PM
Quuxplusone planned changes to this revision.Jan 10 2022, 12:43 PM

Actually, this should probably also use @ldionne's alternative libcxx/test/libcxx/*.sh.py approach from D116958, rather than complicating graph_header_deps.py.
I'll commit the NFC libcxx/include/ changes soon, and then quite possibly roll the Python part into D116958 once I figure that out.

This revision was not accepted when it landed; it landed in state Changes Planned.Jan 10 2022, 1:31 PM
This revision was automatically updated to reflect the committed changes.