Skip to content

[libc++] Remove <tuple> include from <__format/concepts.h> #80214

New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

Sign up for GitHub

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Merged
merged 1 commit into from
Mar 14, 2024
Merged

[libc++] Remove <tuple> include from <__format/concepts.h> #80214

Show file tree
Hide file tree
Changes from all commits
Commits
Show all changes
1 commit
Select commit
File filter

Filter by extension

Filter by extension
Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
4 changes: 3 additions & 1 deletion libcxx/include/__format/concepts.h
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -15,10 +15,12 @@
#include <__config>
#include <__format/format_parse_context.h>
#include <__fwd/format.h>
#include <__fwd/tuple.h>
#include <__tuple/tuple_size.h>
#include <__type_traits/is_specialization.h>
#include <__type_traits/remove_const.h>
#include <__type_traits/remove_reference.h>
#include <__utility/pair.h>
#include <tuple>

#if !defined(_LIBCPP_HAS_NO_PRAGMA_SYSTEM_HEADER)
# pragma GCC system_header
Expand Down
7 changes: 6 additions & 1 deletion libcxx/include/__tuple/tuple_size.h
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -43,7 +43,7 @@ struct _LIBCPP_TEMPLATE_VIS tuple_size<__enable_if_tuple_size_imp< volatile _Tp,

template <class _Tp>
struct _LIBCPP_TEMPLATE_VIS
tuple_size<__enable_if_tuple_size_imp< const volatile _Tp, integral_constant<size_t, sizeof(tuple_size<_Tp>)>>>
tuple_size<__enable_if_tuple_size_imp<const volatile _Tp, integral_constant<size_t, sizeof(tuple_size<_Tp>)>>>
: public integral_constant<size_t, tuple_size<_Tp>::value> {};

#else
Expand All @@ -63,6 +63,11 @@ struct _LIBCPP_TEMPLATE_VIS tuple_size<tuple<_Tp...> > : public integral_constan
template <class... _Tp>
struct _LIBCPP_TEMPLATE_VIS tuple_size<__tuple_types<_Tp...> > : public integral_constant<size_t, sizeof...(_Tp)> {};

# if _LIBCPP_STD_VER >= 17
template <class _Tp>
inline constexpr size_t tuple_size_v = tuple_size<_Tp>::value;
# endif

Comment on lines +66 to +70
Copy link
Member

Choose a reason for hiding this comment

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

Can you mention this in the description?

#endif // _LIBCPP_CXX03_LANG

_LIBCPP_END_NAMESPACE_STD
Expand Down
1 change: 1 addition & 0 deletions libcxx/include/chrono
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -878,6 +878,7 @@ constexpr chrono::year operator ""y(unsigned lo
# include <cstring>
# include <forward_list>
# include <string>
# include <tuple>
#endif

#if !defined(_LIBCPP_REMOVE_TRANSITIVE_INCLUDES) && _LIBCPP_STD_VER == 20
Expand Down
3 changes: 0 additions & 3 deletions libcxx/include/tuple
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -1402,9 +1402,6 @@ inline _LIBCPP_HIDE_FROM_ABI _LIBCPP_CONSTEXPR_SINCE_CXX20 pair<_T1, _T2>::pair(
second(std::forward<_Args2>(std::get<_I2>(__second_args))...) {}

# if _LIBCPP_STD_VER >= 17
template <class _Tp>
inline constexpr size_t tuple_size_v = tuple_size<_Tp>::value;

# define _LIBCPP_NOEXCEPT_RETURN(...) \
noexcept(noexcept(__VA_ARGS__)) { return __VA_ARGS__; }

Expand Down
1 change: 1 addition & 0 deletions libcxx/include/vector
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -2990,6 +2990,7 @@ _LIBCPP_POP_MACROS
# include <concepts>
# include <cstdlib>
# include <locale>
# include <tuple>
Copy link
Member

Choose a reason for hiding this comment

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

I added the tuple dependency for C++20 and now we need to keep it for C++11 - C++17 where tuple was never used. IMO a good reason to discuss our policy.

# include <type_traits>
# include <typeinfo>
# include <utility>
Expand Down
1 change: 0 additions & 1 deletion libcxx/test/libcxx/transitive_includes/cxx23.csv
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -85,7 +85,6 @@ chrono sstream
chrono stdexcept
chrono string
chrono string_view
chrono tuple
Copy link
Member

Choose a reason for hiding this comment

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

AFAIK we wanted to keep our transitive set stable. Maybe we should discuss this policy during a monthly meeting. Based on the several remarks on the size by the Chromium team and recently #80196 I think we should revise our header stability policy or at least have a plan how to remove transitive includes.

@vitaut recently blogged about this too, and _LIBCPP_REMOVE_TRANSITIVE_INCLUDES does not seem to be well known.

@ldionne can you put this on the agenda?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Until now we've only kept the includes up to C++20 stable. C++23 was always fair game. I agree that we should rethink our policy on this, since the amount of transitive includes that we could avoid has become massive. I'm currently also working on a patch to reduce the transitive includes further by including things in the top level headers by version.

Copy link
Member

Choose a reason for hiding this comment

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

Until now we've only kept the includes up to C++20 stable. C++23 was always fair game. I agree that we should rethink our policy on this, since the amount of transitive includes that we could avoid has become massive.

I know, but at some point it was said only the C++ development version is fair game, which would mean C++26. Hence the proposal to discuss this. I was already considering to put it on the agenda after a private conversation with Victor.

I'm currently also working on a patch to reduce the transitive includes further by including things in the top level headers by version.

The last time I had a patch to improve the transitive includes by #ifdef-ing it got a lukewarm response. So nice to hear the response now is better.

Copy link
Member

Choose a reason for hiding this comment

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

Based on the last monthly meeting we didn't declare C++23 includes frozen.

chrono vector
chrono version
cinttypes cstdint
Expand Down
1 change: 0 additions & 1 deletion libcxx/test/libcxx/transitive_includes/cxx26.csv
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -85,7 +85,6 @@ chrono sstream
chrono stdexcept
chrono string
chrono string_view
chrono tuple
chrono vector
chrono version
cinttypes cstdint
Expand Down
8 changes: 4 additions & 4 deletions libcxx/test/std/utilities/tuple/tuple.tuple/tuple.helper/tuple_size_v.verify.cpp
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -17,8 +17,8 @@
#include <tuple>

void f() {
(void)std::tuple_size_v<std::tuple<> &>; // expected-note {{requested here}}
(void)std::tuple_size_v<int>; // expected-note {{requested here}}
(void)std::tuple_size_v<std::tuple<>*>; // expected-note {{requested here}}
// expected-error@tuple:* 3 {{implicit instantiation of undefined template}}
(void)std::tuple_size_v<std::tuple<>&>; // expected-note {{requested here}}
(void)std::tuple_size_v<int>; // expected-note {{requested here}}
(void)std::tuple_size_v<std::tuple<>*>; // expected-note {{requested here}}
// expected-error@*:* 3 {{implicit instantiation of undefined template}}
}