[libc++][format] Fixes vector<bool> requirements.

[Mordante]

Summary

Makes sure the formatter for the vector<bool>::reference is enabled
when only the header <vector> is included. Before this change it
required <vector> and <format> to be included. This violated the
requirements in the Standard.

Fixes: https://llvm.org/PR61314

[Mordante]

Rebased and CI fixes.

[Mordante]

CI fixes.

[Mordante]

CI fixes.

[Mordante]

Fixes CI.

[ldionne]

LGTM but I think we should probably remove the header workaround.

libcxx/include/vector

325

Which ones would we need to remove in order not to have this workaround? I think it might be reasonable to take the hit -- usually our stance is that we try not to remove transitive includes just for the sake of it, however when those transitive includes are a blocker to our work, we remove them. So IMO it would be reasonable to remove the transitive includes causing problems here, unless the impact is really too big.

[Mordante]

Rebased and addresses review comment.

[Mordante]

Rebased and trigger CI.

[Mordante]

CI fixes.

[Mordante]

Rebased and trigger CI.

[hans]

In Chromium we noticed that this almost doubled the preprocessed size of <vector>, from ca 1.6 MB to 3.2 MB. Since it's a widely included header, that results in ca 8 GB (2.5%) of extra code to compile during a full build.

I realize this change is based on a requirement in the standard, but it seems unfortunate that the cost of the format library is spilling over to vector like this. If there's anything that could be done to reduce the impact here, that would be very valuable to us and presumably other libc++ users too.

[Mordante]

In D149543#4455780, @hans wrote:

In Chromium we noticed that this almost doubled the preprocessed size of <vector>, from ca 1.6 MB to 3.2 MB. Since it's a widely included header, that results in ca 8 GB (2.5%) of extra code to compile during a full build.

I realize this change is based on a requirement in the standard, but it seems unfortunate that the cost of the format library is spilling over to vector like this. If there's anything that could be done to reduce the impact here, that would be very valuable to us and presumably other libc++ users too.

Interesting. Am I right to assume most of the new size is due to including <string>? 2.5% is quite a bit. I'm not sure whether there is a good solution. (Unfortunately modules are not really usable yet.)

I have verified and the bool formatter really needs a string. It needs to use truename which is a string
https://en.cppreference.com/w/cpp/locale/numpunct/truefalsename

[hans]

In D149543#4456497, @Mordante wrote:

Interesting. Am I right to assume most of the new size is due to including <string>?

I don't think it's <string>; that's already widely included. I think it's __format/formatter_output.h where most of it gets added.

Part of me thinks maybe <format> could be gated behind a macro like is was while it was experimental, since we're not currently using it in Chromium. But that would only delay the issue until we do want to use it of course.

[ldionne]

In D149543#4456976, @hans wrote:

In D149543#4456497, @Mordante wrote:

Interesting. Am I right to assume most of the new size is due to including <string>?

I don't think it's <string>; that's already widely included. I think it's __format/formatter_output.h where most of it gets added.

@Mordante What's the very very minimum amount of stuff we need to implement formatter<bool>? Are we somehow including a lot more than we strictly need? Can we split up some of those format headers to avoid including some of this stuff transitively?

Part of me thinks maybe <format> could be gated behind a macro like is was while it was experimental, since we're not currently using it in Chromium. But that would only delay the issue until we do want to use it of course.

IMO that's just sweeping the issue under the carpet.

[Mordante]

In D149543#4459956, @ldionne wrote:

In D149543#4456976, @hans wrote:

In D149543#4456497, @Mordante wrote:

Interesting. Am I right to assume most of the new size is due to including <string>?

I don't think it's <string>; that's already widely included. I think it's __format/formatter_output.h where most of it gets added.

Thanks for the info @hans.

@Mordante What's the very very minimum amount of stuff we need to implement formatter<bool>? Are we somehow including a lot more than we strictly need? Can we split up some of those format headers to avoid including some of this stuff transitively?

At the moment we include the minimum set of granularized headers we need. I had a look yesterday and I found some unneeded includes in our granularized format headers, I'll create a patch for that after testing. I think I can slice up formatter_output.h and reduce the number includes that way. I'll look into that later this week.

Part of me thinks maybe <format> could be gated behind a macro like is was while it was experimental, since we're not currently using it in Chromium. But that would only delay the issue until we do want to use it of course.

IMO that's just sweeping the issue under the carpet.

I agree. However I think the performance of compiling vector is important. So I'm not entirely against it as long as users (chromium) explicitly needs to opt out of it, so by default you get the the current behaviour.

[huihuiz]

This is causing compilation failure in https://github.com/llvm/llvm-project/issues/64971

libcxx/include/vector

 #include <__algorithm/rotate.h>
 #include <__algorithm/unwrap_iter.h>
 #include <__assert> // all public C++ headers provide the assertion handler
 #include <__bit_reference>
 #include <__concepts/same_as.h>
 #include <__config>
 #include <__debug>
 #include <__format/enable_insertable.h>
-#include <__format/formatter.h>
 #include <__functional/hash.h>
 #include <__functional/unary_function.h>
 #include <__iterator/advance.h>
 #include <__iterator/iterator_traits.h>
 #include <__iterator/reverse_iterator.h>
 #include <__iterator/wrap_iter.h>
 #include <__memory/addressof.h>
 #include <__memory/allocate_at_least.h>

 #include <__utility/swap.h>
 #include <climits>
 #include <cstring>
 #include <iosfwd> // for forward declaration of vector
 #include <limits>
 #include <stdexcept>
 #include <version>
 
+// TODO FMT Make these normal includes after transitive includes are
+// removed from older language versions.
+#if !defined(_LIBCPP_HAS_NO_INCOMPLETE_FORMAT) && _LIBCPP_STD_VER >= 23
+#  include <__format/formatter.h>
+#  include <__format/formatter_bool.h>
+#endif
+
 // standard-mandated includes
 
 // [iterator.range]
 #include <__iterator/access.h>
 #include <__iterator/data.h>
 #include <__iterator/empty.h>
 #include <__iterator/reverse_access.h>
 #include <__iterator/size.h>

 inline constexpr bool __format::__enable_insertable<vector<char>> = true;
 #ifndef _LIBCPP_HAS_NO_WIDE_CHARACTERS
 template <>
 inline constexpr bool __format::__enable_insertable<vector<wchar_t>> = true;
 #endif
 
 #endif // _LIBCPP_STD_VER >= 20
 
-#if _LIBCPP_STD_VER >= 23
+#if !defined(_LIBCPP_HAS_NO_INCOMPLETE_FORMAT) && _LIBCPP_STD_VER >= 23
 template <class _Tp, class CharT>
 // Since is-vector-bool-reference is only used once it's inlined here.
   requires same_as<typename _Tp::__container, vector<bool, typename _Tp::__container::allocator_type>>
 struct _LIBCPP_TEMPLATE_VIS formatter<_Tp, CharT> {
 private:
   formatter<bool, CharT> __underlying_;
 
 public:
   template <class _ParseContext>
   _LIBCPP_HIDE_FROM_ABI constexpr typename _ParseContext::iterator parse(_ParseContext& __ctx) {
         return __underlying_.parse(__ctx);
   }
 
   template <class _FormatContext>
   _LIBCPP_HIDE_FROM_ABI typename _FormatContext::iterator format(const _Tp& __ref, _FormatContext& __ctx) const {
         return __underlying_.format(__ref, __ctx);
   }
 };
-#endif // _LIBCPP_STD_VER >= 23
+#endif // !defined(_LIBCPP_HAS_NO_INCOMPLETE_FORMAT) && _LIBCPP_STD_VER >= 23
 
 _LIBCPP_END_NAMESPACE_STD
 
 #if _LIBCPP_STD_VER >= 17
 _LIBCPP_BEGIN_NAMESPACE_STD
 namespace pmr {
 template <class _ValueT>
 using vector = std::vector<_ValueT, polymorphic_allocator<_ValueT>>;

libcxx/test/libcxx/transitive_includes/cxx2b.csv

 variant cstddef
 variant cstdint
 variant cstring
 variant initializer_list
 variant limits
 variant new
 variant tuple
 variant version
+vector array
+vector cerrno
 vector climits
 vector compare
 vector cstddef
 vector cstdint
+vector cstdlib
 vector cstring
 vector initializer_list
 vector iosfwd
 vector limits
+vector locale
 vector new
 vector stdexcept
+vector string
+vector string_view
 vector tuple
 vector version

libcxx/test/std/containers/sequences/vector.bool/vector.bool.fmt/types.compile.pass.cpp

+//===----------------------------------------------------------------------===//
+// Part of the LLVM Project, under the Apache License v2.0 with LLVM Exceptions.
+// See https://llvm.org/LICENSE.txt for license information.
+// SPDX-License-Identifier: Apache-2.0 WITH LLVM-exception
+//
+//===----------------------------------------------------------------------===//
+
+// UNSUPPORTED: c++03, c++11, c++14, c++17, c++20
+// UNSUPPORTED: libcpp-has-no-incomplete-format
+
+// <vector>
+
+// template<class T, class charT> requires is-vector-bool-reference<T>
+//  struct formatter<T, charT>;
+
+// [format.formatter.spec]/4
+//   If the library provides an explicit or partial specialization of
+//   formatter<T, charT>, that specialization is enabled and meets the
+//   Formatter requirements except as noted otherwise.
+//
+// Tests parts of the BasicFormatter requirements. Like the formattable concept
+// it uses the semiregular concept. It test does not use the formattable
+// concept since it is the intention the formatter is available without
+// including the format header.
+
+// TODO FMT Evaluate what to do with [format.formatter.spec]/2
+// [format.formatter.spec]/2
+//   Each header that declares the template formatter provides the following
+//   enabled specializations:
+// Then there is a list of formatters, but is that really useful?
+// Note this should be discussed in LEWG.
+
+#include <concepts>
+#include <vector>
+
+#include "test_macros.h"
+
+static_assert(std::semiregular<std::formatter<std::vector<bool>::reference, char>>);
+#ifndef TEST_HAS_NO_WIDE_CHARACTERS
+static_assert(std::semiregular<std::formatter<std::vector<bool>::reference, wchar_t>>);
+#endif