Skip to content

Conversation

@atetubou
Copy link
Contributor

@atetubou atetubou requested a review from a team as a code owner October 17, 2024 08:13
@llvmbot llvmbot added the libc++ libc++ C++ Standard Library. Not GNU libstdc++. Not libc++abi. label Oct 17, 2024
@llvmbot
Copy link
Member

llvmbot commented Oct 17, 2024

@llvm/pr-subscribers-libcxx

Author: Takuto Ikuta (atetubou)

Changes

This is to reduce included header file size in pre C++23 build.

e.g. 2% of total included file coming from formatter_bool.h in chrome build.
https://commondatastorage.googleapis.com/chromium-browser-clang/chrome_includes_2024-10-17_004801.html#view=edges&filter=&sort=asize&reverse=&includer=%5Ethird_party%2Flibc%5C%2B%5C%2B%2Fsrc%2Finclude%2Fvector%24&included=&limit=1000


Full diff: https://github.com/llvm/llvm-project/pull/112675.diff

1 Files Affected:

  • (modified) libcxx/include/vector (+5-2)
diff --git a/libcxx/include/vector b/libcxx/include/vector
index dc31f31838264c..93755afdde711c 100644
--- a/libcxx/include/vector
+++ b/libcxx/include/vector
@@ -324,8 +324,6 @@ template<class T, class charT> requires is-vector-bool-reference<T> // Since C++
 #include <__config>
 #include <__debug_utils/sanitizers.h>
 #include <__format/enable_insertable.h>
-#include <__format/formatter.h>
-#include <__format/formatter_bool.h>
 #include <__functional/hash.h>
 #include <__functional/unary_function.h>
 #include <__fwd/vector.h>
@@ -391,6 +389,11 @@ template<class T, class charT> requires is-vector-bool-reference<T> // Since C++
 #include <compare>
 #include <initializer_list>
 
+#if _LIBCPP_STD_VER >= 23
+#  include <__format/formatter.h>
+#  include <__format/formatter_bool.h>
+#endif
+
 #if !defined(_LIBCPP_HAS_NO_PRAGMA_SYSTEM_HEADER)
 #  pragma GCC system_header
 #endif

@atetubou atetubou force-pushed the include_formatter_only_in_23 branch from 03ddb1b to 1776748 Compare October 17, 2024 08:59
Copy link
Contributor

@frederick-vs-ja frederick-vs-ja left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Perhaps we should correspondingly remove related entries in cxx11.csv ~ cxx20.csv in libcxx/test/libcxx/transitive_includes.

@zmodem
Copy link
Collaborator

zmodem commented Oct 17, 2024

There was some discussion of this in https://reviews.llvm.org/D149543 with @mordante

I'm not familiar with the standard's requirements, but if they're only needed only in C++23, making the includes conditional sounds like a good option.

@atetubou atetubou force-pushed the include_formatter_only_in_23 branch from 1776748 to dc18532 Compare October 18, 2024 04:21
@atetubou
Copy link
Contributor Author

I updated libcxx/test/libcxx/transitive_includes for some C++ versions.

From my understanding, formatter headers are only used from C++23's code for vector<bool> and we don't need to include then in earlier version.

Copy link
Contributor

@philnik777 philnik777 left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

See #99705

@atetubou
Copy link
Contributor Author

@philnik777 oh thanks, will you update the PR to rebase and pass the tests?

@frederick-vs-ja
Copy link
Contributor

This PR seems to be superseded. Is there any additional necessary work?

@atetubou
Copy link
Contributor Author

No, let me close this PR. Thank you for your reminder.

@atetubou atetubou closed this Oct 31, 2024
@atetubou atetubou deleted the include_formatter_only_in_23 branch October 31, 2024 11:10
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

libc++ libc++ C++ Standard Library. Not GNU libstdc++. Not libc++abi.

Projects

None yet

Development

Successfully merging this pull request may close these issues.

5 participants