[libcxx-commits] [PATCH] D132284: [libc++] Reduces the number of transitive includes.
Mark de Wever via Phabricator via libcxx-commits
libcxx-commits at lists.llvm.org
Thu Aug 25 10:30:01 PDT 2022
Mordante created this revision.
Herald added a project: All.
Mordante updated this revision to Diff 454178.
Mordante added a comment.
Herald added a subscriber: arichardson.
Mordante updated this revision to Diff 454184.
Mordante updated this revision to Diff 454190.
Mordante updated this revision to Diff 454283.
Herald added subscribers: miyuki, arphaman.
Mordante updated this revision to Diff 454284.
Mordante updated this revision to Diff 454298.
Mordante updated this revision to Diff 454879.
Mordante updated this revision to Diff 454903.
Mordante updated this revision to Diff 455083.
Mordante updated this revision to Diff 455144.
Mordante updated this revision to Diff 455283.
Herald added a subscriber: mgorny.
Mordante edited the summary of this revision.
Mordante edited the summary of this revision.
Mordante updated this revision to Diff 455641.
Mordante marked 9 inline comments as done.
Mordante edited the summary of this revision.
Mordante published this revision for review.
Herald added a project: libc++.
Herald added a subscriber: libcxx-commits.
Herald added a reviewer: libc++.
Test.
Mordante added a comment.
Change the includes.
Mordante added a comment.
Test
Mordante added a comment.
Polishing, adding documentation, fixing some CI issues.
Mordante added a comment.
Fix CI.
Mordante added a comment.
Attempt to fix Windows.
ldionne added a comment.
I think this is a great step forward, but it does raise some interesting/annoying questions. I think we can solve all of them, though.
Mordante added a comment.
Test Windows build based on ldionne's suggestion.
Mordante added a comment.
Try to fix the build.
Mordante added a comment.
Fix CI.
Mordante added a comment.
Fix other Windows build.
Mordante added a comment.
Test another way to fix the Windows build.
Mordante added a comment.
Addresses review comments.
Removes unrelated changes, these have been moved to other patches.
Rebased on top of D132584 <https://reviews.llvm.org/D132584>, to include the new transitve include test framework.
================
Comment at: libcxx/docs/DesignDocs/HeaderRemovalPolicy.rst:18-20
+The benefit of using minimal headers is that the size of libc++'s top-level
+headers becomes smaller. This improves the compilation time when users include
+a top-level header.
----------------
================
Comment at: libcxx/docs/DesignDocs/HeaderRemovalPolicy.rst:22-25
+A disadvantage is that users unknowingly depend on these transitive includes.
+Thus removing an include might break their build after upgrading a newer
+version of libc++. For example ``<algorithm>`` is a header that is not
+included often, but it happens to work.
----------------
================
Comment at: libcxx/docs/DesignDocs/HeaderRemovalPolicy.rst:27-31
+To avoid breaking users libc++ will not remove headers from a top-level header
+of released C++ versions. Libc++ will only remove headers for the next language
+version; that is the version the C++ committee is working on. After C++23
+becomes an ISO Standard libc++ will no longer remove headers from C++23.
+Instead when removing a header it will be done in C++26.
----------------
================
Comment at: libcxx/docs/DesignDocs/HeaderRemovalPolicy.rst:33
+
+The removal is done by the following code for C++23.
+
----------------
================
Comment at: libcxx/docs/DesignDocs/HeaderRemovalPolicy.rst:44-47
+When users define ``_LIBCPP_REMOVE_TRANSITIVE_INCLUDES`` it will not include
+these headers regardless of the language version. This aids users to transition
+to a newer language version. Secondly it aids users who want to use smaller
+headers in older language versions.
----------------
================
Comment at: libcxx/docs/DesignDocs/HeaderRemovalPolicy.rst:49
+
+One of the issues for libc++ with transitive includes is that these includes
+may create dependency cycles and cause the validation script
----------------
Instead of this, I would suggest:
1. Adding a new `libcxx-generate-public-header-graph` job that produces all the `expected.FOO` headers using `clang --trace-includes`
2. This should allow getting rid of `regenerate_expected_results = False/True` in `libcxx/test/libcxx/transitive_includes.sh.cpp`.
3. Rewrite `graph_header_deps.py` to use that data instead of the naive `grep`-based parsing it has right now.
This is fine to do as a follow-up.
================
Comment at: libcxx/docs/ReleaseNotes.rst:48-62
+- Several unneeded includes have been removed from libc++. The headers are
+ removed based on the language version used. The following headers have been
+ removed when not needed:
+
+ - C++20: ``chrono``
+ - C++2b: ``algorithm``, ``array``, ``atomic``, ``bit``, ``chrono``,
+ ``climits``, ``cmath``, ``compare``, ``concepts``, ``cstdlib``,
----------------
================
Comment at: libcxx/include/algorithm:1902
-#ifndef _LIBCPP_REMOVE_TRANSITIVE_INCLUDES
-# include <chrono>
+#if !defined(_LIBCPP_REMOVE_TRANSITIVE_INCLUDES) && _LIBCPP_STD_VER <= 17
+# include <chrono> // IGNORE-CYCLE due to <format>
----------------
During the live code review we discussed to move these transitive includes to the end of the file. That will be done in a followup patch.
================
Comment at: libcxx/include/chrono:686
+// Guarded by language version to avoid include cycles prior to C++20.
+// (In C++20 it's solved by removing transitive headers.)
----------------
I would like to investigate this instead:
```
#if _BUILDING_ON_WINDOWS_DLL // don't require usage of libexperimental cause it doesn't work yet
virtual ~format_error() noexcept { }
#endif
```
I'm not sure it will be better, but we should try it out. It seems more localized to me.
================
Comment at: libcxx/include/chrono:686
+// Guarded by language version to avoid include cycles prior to C++20.
+// (In C++20 it's solved by removing transitive headers.)
----------------
ldionne wrote:
> I would like to investigate this instead:
>
> ```
> #if _BUILDING_ON_WINDOWS_DLL // don't require usage of libexperimental cause it doesn't work yet
> virtual ~format_error() noexcept { }
> #endif
> ```
>
> I'm not sure it will be better, but we should try it out. It seems more localized to me.
Fixed in D132667 in the suggested way. The changes are removed here.
================
Comment at: libcxx/include/chrono:692
+# include <format>
+#endif
+
----------------
Note this doesn't belong in this patch. It's a test to see whether we can fix the build issues in D128577.
================
Comment at: libcxx/test/libcxx/transitive_includes/expected.algorithm:1
algorithm
atomic
----------------
This is extremely unfortunate, but I think that we *have* to generate these headers for each language version if we move forward with this patch. Otherwise, we defeat the whole purpose of having this framework in place to prevent transitive include removal regressions.
================
Comment at: libcxx/test/libcxx/transitive_includes/expected.algorithm:1
algorithm
atomic
----------------
ldionne wrote:
> This is extremely unfortunate, but I think that we *have* to generate these headers for each language version if we move forward with this patch. Otherwise, we defeat the whole purpose of having this framework in place to prevent transitive include removal regressions.
Solved in D132534 and D132584.
================
Comment at: libcxx/test/libcxx/transitive_includes/expected.algorithm:17
iosfwd
-iterator
limits
----------------
We should keep the C++20 removals as minimal as possible. You should keep including `<iterator>`, `<utility>` and `<variant>` explicitly from `<algorithm>` in <= C++20, but drop them in C++23.
================
Comment at: libcxx/utils/ci/run-buildbot:570
# setting when cmake and the test driver does the right thing automatically.
- generate-cmake-libcxx-win -DLIBCXX_TEST_PARAMS="enable_experimental=False"
+ generate-cmake-libcxx-win -DLIBCXX_TEST_PARAMS="enable_experimental=False" \
+ -DLIBCXX_EXTRA_SITE_DEFINES="_LIBCPP_HAS_NO_INT128;_LIBCPP_INLINE_FORMAT_ERROR_DTOR" # TODO FMT Remove when format becomes mainline.
----------------
I think we should define `_LIBCPP_INLINE_FORMAT_ERROR_DTOR` inside `__config` instead. This is a general property of clang-cl / DLL builds, not only our CI build. IOW, we should enable the `inline` dtor if someone builds DLLs using clang-cl at their desk, which wouldn't be the case right now.
This defines a new policy for removal of transitive includes.
The goal of the policy it to make it relatively easy to remove
headers when needed, but avoid breaking developers using and
vendors shipping libc++.
The method used is to guard transitive includes based on the
C++ language version. For the upcoming C++23 we can remove
headers when we want, but for other language versions we try
to keep it to a minimum.
In this code the transitive include of `<chrono>` is removed
since D128577 <https://reviews.llvm.org/D128577> introduces a header cycle between `<format>`
and `<chrono>`. This cycle is indirectly required by the
Standard. Our cycle dependency tool basically is a grep based
tool, so it needs some hints to ignore cycles. With the input
of our transitive include tests we can create a better tool.
However that's out of the scope of this patch.
Note the flag `_LIBCPP_REMOVE_TRANSITIVE_INCLUDES` remains
unchanged. So users can still opt-out of transitives includes
entirely.
Repository:
rG LLVM Github Monorepo
https://reviews.llvm.org/D132284
Files:
libcxx/docs/DesignDocs/HeaderRemovalPolicy.rst
libcxx/docs/ReleaseNotes.rst
libcxx/docs/index.rst
libcxx/include/algorithm
libcxx/include/any
libcxx/include/array
libcxx/include/atomic
libcxx/include/bit
libcxx/include/charconv
libcxx/include/coroutine
libcxx/include/deque
libcxx/include/experimental/simd
libcxx/include/experimental/unordered_map
libcxx/include/ext/hash_map
libcxx/include/ext/hash_set
libcxx/include/forward_list
libcxx/include/functional
libcxx/include/future
libcxx/include/iterator
libcxx/include/list
libcxx/include/locale
libcxx/include/map
libcxx/include/memory
libcxx/include/mutex
libcxx/include/numeric
libcxx/include/optional
libcxx/include/ostream
libcxx/include/random
libcxx/include/regex
libcxx/include/set
libcxx/include/span
libcxx/include/stack
libcxx/include/string
libcxx/include/string_view
libcxx/include/thread
libcxx/include/tuple
libcxx/include/typeindex
libcxx/include/unordered_map
libcxx/include/unordered_set
libcxx/include/utility
libcxx/include/valarray
libcxx/include/variant
libcxx/include/vector
libcxx/test/libcxx/transitive_includes/cxx20/expected.algorithm
libcxx/test/libcxx/transitive_includes/cxx20/expected.any
libcxx/test/libcxx/transitive_includes/cxx20/expected.array
libcxx/test/libcxx/transitive_includes/cxx20/expected.atomic
libcxx/test/libcxx/transitive_includes/cxx20/expected.barrier
libcxx/test/libcxx/transitive_includes/cxx20/expected.bitset
libcxx/test/libcxx/transitive_includes/cxx20/expected.ccomplex
libcxx/test/libcxx/transitive_includes/cxx20/expected.codecvt
libcxx/test/libcxx/transitive_includes/cxx20/expected.complex
libcxx/test/libcxx/transitive_includes/cxx20/expected.condition_variable
libcxx/test/libcxx/transitive_includes/cxx20/expected.ctgmath
libcxx/test/libcxx/transitive_includes/cxx20/expected.deque
libcxx/test/libcxx/transitive_includes/cxx20/expected.experimental_algorithm
libcxx/test/libcxx/transitive_includes/cxx20/expected.experimental_coroutine
libcxx/test/libcxx/transitive_includes/cxx20/expected.experimental_deque
libcxx/test/libcxx/transitive_includes/cxx20/expected.experimental_forward_list
libcxx/test/libcxx/transitive_includes/cxx20/expected.experimental_functional
libcxx/test/libcxx/transitive_includes/cxx20/expected.experimental_list
libcxx/test/libcxx/transitive_includes/cxx20/expected.experimental_map
libcxx/test/libcxx/transitive_includes/cxx20/expected.experimental_memory_resource
libcxx/test/libcxx/transitive_includes/cxx20/expected.experimental_regex
libcxx/test/libcxx/transitive_includes/cxx20/expected.experimental_set
libcxx/test/libcxx/transitive_includes/cxx20/expected.experimental_simd
libcxx/test/libcxx/transitive_includes/cxx20/expected.experimental_string
libcxx/test/libcxx/transitive_includes/cxx20/expected.experimental_unordered_map
libcxx/test/libcxx/transitive_includes/cxx20/expected.experimental_unordered_set
libcxx/test/libcxx/transitive_includes/cxx20/expected.experimental_vector
libcxx/test/libcxx/transitive_includes/cxx20/expected.ext_hash_map
libcxx/test/libcxx/transitive_includes/cxx20/expected.ext_hash_set
libcxx/test/libcxx/transitive_includes/cxx20/expected.filesystem
libcxx/test/libcxx/transitive_includes/cxx20/expected.format
libcxx/test/libcxx/transitive_includes/cxx20/expected.forward_list
libcxx/test/libcxx/transitive_includes/cxx20/expected.fstream
libcxx/test/libcxx/transitive_includes/cxx20/expected.functional
libcxx/test/libcxx/transitive_includes/cxx20/expected.future
libcxx/test/libcxx/transitive_includes/cxx20/expected.iomanip
libcxx/test/libcxx/transitive_includes/cxx20/expected.ios
libcxx/test/libcxx/transitive_includes/cxx20/expected.iostream
libcxx/test/libcxx/transitive_includes/cxx20/expected.istream
libcxx/test/libcxx/transitive_includes/cxx20/expected.latch
libcxx/test/libcxx/transitive_includes/cxx20/expected.list
libcxx/test/libcxx/transitive_includes/cxx20/expected.locale
libcxx/test/libcxx/transitive_includes/cxx20/expected.map
libcxx/test/libcxx/transitive_includes/cxx20/expected.memory
libcxx/test/libcxx/transitive_includes/cxx20/expected.mutex
libcxx/test/libcxx/transitive_includes/cxx20/expected.numeric
libcxx/test/libcxx/transitive_includes/cxx20/expected.optional
libcxx/test/libcxx/transitive_includes/cxx20/expected.ostream
libcxx/test/libcxx/transitive_includes/cxx20/expected.queue
libcxx/test/libcxx/transitive_includes/cxx20/expected.random
libcxx/test/libcxx/transitive_includes/cxx20/expected.ranges
libcxx/test/libcxx/transitive_includes/cxx20/expected.regex
libcxx/test/libcxx/transitive_includes/cxx20/expected.scoped_allocator
libcxx/test/libcxx/transitive_includes/cxx20/expected.semaphore
libcxx/test/libcxx/transitive_includes/cxx20/expected.set
libcxx/test/libcxx/transitive_includes/cxx20/expected.shared_mutex
libcxx/test/libcxx/transitive_includes/cxx20/expected.span
libcxx/test/libcxx/transitive_includes/cxx20/expected.sstream
libcxx/test/libcxx/transitive_includes/cxx20/expected.stack
libcxx/test/libcxx/transitive_includes/cxx20/expected.streambuf
libcxx/test/libcxx/transitive_includes/cxx20/expected.string
libcxx/test/libcxx/transitive_includes/cxx20/expected.string_view
libcxx/test/libcxx/transitive_includes/cxx20/expected.strstream
libcxx/test/libcxx/transitive_includes/cxx20/expected.system_error
libcxx/test/libcxx/transitive_includes/cxx20/expected.thread
libcxx/test/libcxx/transitive_includes/cxx20/expected.unordered_map
libcxx/test/libcxx/transitive_includes/cxx20/expected.unordered_set
libcxx/test/libcxx/transitive_includes/cxx20/expected.valarray
libcxx/test/libcxx/transitive_includes/cxx20/expected.vector
libcxx/test/libcxx/transitive_includes/cxx2b/expected.algorithm
libcxx/test/libcxx/transitive_includes/cxx2b/expected.any
libcxx/test/libcxx/transitive_includes/cxx2b/expected.array
libcxx/test/libcxx/transitive_includes/cxx2b/expected.atomic
libcxx/test/libcxx/transitive_includes/cxx2b/expected.barrier
libcxx/test/libcxx/transitive_includes/cxx2b/expected.bit
libcxx/test/libcxx/transitive_includes/cxx2b/expected.bitset
libcxx/test/libcxx/transitive_includes/cxx2b/expected.ccomplex
libcxx/test/libcxx/transitive_includes/cxx2b/expected.charconv
libcxx/test/libcxx/transitive_includes/cxx2b/expected.codecvt
libcxx/test/libcxx/transitive_includes/cxx2b/expected.complex
libcxx/test/libcxx/transitive_includes/cxx2b/expected.condition_variable
libcxx/test/libcxx/transitive_includes/cxx2b/expected.coroutine
libcxx/test/libcxx/transitive_includes/cxx2b/expected.ctgmath
libcxx/test/libcxx/transitive_includes/cxx2b/expected.deque
libcxx/test/libcxx/transitive_includes/cxx2b/expected.experimental_algorithm
libcxx/test/libcxx/transitive_includes/cxx2b/expected.experimental_coroutine
libcxx/test/libcxx/transitive_includes/cxx2b/expected.experimental_deque
libcxx/test/libcxx/transitive_includes/cxx2b/expected.experimental_forward_list
libcxx/test/libcxx/transitive_includes/cxx2b/expected.experimental_functional
libcxx/test/libcxx/transitive_includes/cxx2b/expected.experimental_iterator
libcxx/test/libcxx/transitive_includes/cxx2b/expected.experimental_list
libcxx/test/libcxx/transitive_includes/cxx2b/expected.experimental_map
libcxx/test/libcxx/transitive_includes/cxx2b/expected.experimental_memory_resource
libcxx/test/libcxx/transitive_includes/cxx2b/expected.experimental_regex
libcxx/test/libcxx/transitive_includes/cxx2b/expected.experimental_set
libcxx/test/libcxx/transitive_includes/cxx2b/expected.experimental_simd
libcxx/test/libcxx/transitive_includes/cxx2b/expected.experimental_string
libcxx/test/libcxx/transitive_includes/cxx2b/expected.experimental_unordered_map
libcxx/test/libcxx/transitive_includes/cxx2b/expected.experimental_unordered_set
libcxx/test/libcxx/transitive_includes/cxx2b/expected.experimental_utility
libcxx/test/libcxx/transitive_includes/cxx2b/expected.experimental_vector
libcxx/test/libcxx/transitive_includes/cxx2b/expected.ext_hash_map
libcxx/test/libcxx/transitive_includes/cxx2b/expected.ext_hash_set
libcxx/test/libcxx/transitive_includes/cxx2b/expected.filesystem
libcxx/test/libcxx/transitive_includes/cxx2b/expected.format
libcxx/test/libcxx/transitive_includes/cxx2b/expected.forward_list
libcxx/test/libcxx/transitive_includes/cxx2b/expected.fstream
libcxx/test/libcxx/transitive_includes/cxx2b/expected.functional
libcxx/test/libcxx/transitive_includes/cxx2b/expected.future
libcxx/test/libcxx/transitive_includes/cxx2b/expected.iomanip
libcxx/test/libcxx/transitive_includes/cxx2b/expected.ios
libcxx/test/libcxx/transitive_includes/cxx2b/expected.iostream
libcxx/test/libcxx/transitive_includes/cxx2b/expected.istream
libcxx/test/libcxx/transitive_includes/cxx2b/expected.iterator
libcxx/test/libcxx/transitive_includes/cxx2b/expected.latch
libcxx/test/libcxx/transitive_includes/cxx2b/expected.list
libcxx/test/libcxx/transitive_includes/cxx2b/expected.locale
libcxx/test/libcxx/transitive_includes/cxx2b/expected.map
libcxx/test/libcxx/transitive_includes/cxx2b/expected.memory
libcxx/test/libcxx/transitive_includes/cxx2b/expected.mutex
libcxx/test/libcxx/transitive_includes/cxx2b/expected.numeric
libcxx/test/libcxx/transitive_includes/cxx2b/expected.optional
libcxx/test/libcxx/transitive_includes/cxx2b/expected.ostream
libcxx/test/libcxx/transitive_includes/cxx2b/expected.queue
libcxx/test/libcxx/transitive_includes/cxx2b/expected.random
libcxx/test/libcxx/transitive_includes/cxx2b/expected.ranges
libcxx/test/libcxx/transitive_includes/cxx2b/expected.regex
libcxx/test/libcxx/transitive_includes/cxx2b/expected.scoped_allocator
libcxx/test/libcxx/transitive_includes/cxx2b/expected.semaphore
libcxx/test/libcxx/transitive_includes/cxx2b/expected.set
libcxx/test/libcxx/transitive_includes/cxx2b/expected.shared_mutex
libcxx/test/libcxx/transitive_includes/cxx2b/expected.span
libcxx/test/libcxx/transitive_includes/cxx2b/expected.sstream
libcxx/test/libcxx/transitive_includes/cxx2b/expected.stack
libcxx/test/libcxx/transitive_includes/cxx2b/expected.streambuf
libcxx/test/libcxx/transitive_includes/cxx2b/expected.string
libcxx/test/libcxx/transitive_includes/cxx2b/expected.string_view
libcxx/test/libcxx/transitive_includes/cxx2b/expected.strstream
libcxx/test/libcxx/transitive_includes/cxx2b/expected.system_error
libcxx/test/libcxx/transitive_includes/cxx2b/expected.thread
libcxx/test/libcxx/transitive_includes/cxx2b/expected.tuple
libcxx/test/libcxx/transitive_includes/cxx2b/expected.typeindex
libcxx/test/libcxx/transitive_includes/cxx2b/expected.unordered_map
libcxx/test/libcxx/transitive_includes/cxx2b/expected.unordered_set
libcxx/test/libcxx/transitive_includes/cxx2b/expected.utility
libcxx/test/libcxx/transitive_includes/cxx2b/expected.valarray
libcxx/test/libcxx/transitive_includes/cxx2b/expected.variant
libcxx/test/libcxx/transitive_includes/cxx2b/expected.vector
libcxx/utils/graph_header_deps.py
-------------- next part --------------
A non-text attachment was scrubbed...
Name: D132284.455641.patch
Type: text/x-patch
Size: 91872 bytes
Desc: not available
URL: <http://lists.llvm.org/pipermail/libcxx-commits/attachments/20220825/6bc77fbe/attachment-0001.bin>
More information about the libcxx-commits
mailing list