Skip to content

[WIP] add structured binding forward declaration #127

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.

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

Closed
wants to merge 1 commit into from

Conversation

sthalik
Copy link
Contributor

@sthalik sthalik commented Feb 19, 2022

This commit adds std::tuple_element and std::tuple_size forward declarations to avoid slurping in thousands of lines worth of STL headers. For unsupported STL versions, <array> is included instead.

@sthalik sthalik force-pushed the pr-structured-binding-fwd branch from 83e4819 to 15eb309 Compare February 19, 2022 12:42
@@ -69,6 +69,7 @@ corrade_add_test(ContainersStringViewTest StringViewTest.cpp LIBRARIES CorradeUt
corrade_add_test(ContainersStringViewStlTest StringViewStlTest.cpp)
corrade_add_test(ContainersTripleTest TripleTest.cpp)
corrade_add_test(ContainersTripleStlTest TripleStlTest.cpp)
corrade_add_test(ContainersTupleStlTest TupleStlTest.cpp)
Copy link
Owner

@mosra mosra Feb 19, 2022

Choose a reason for hiding this comment

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

To make the tests on ancient compilers stop failing, move this inside the C++17 if() below, same place where you set the C++17 standard

@sthalik sthalik force-pushed the pr-structured-binding-fwd branch 2 times, most recently from 3697bc1 to f2d56f7 Compare February 19, 2022 12:59
@codecov
Copy link

codecov bot commented Feb 19, 2022

Codecov Report

Base: 97.96% // Head: 97.96% // Increases project coverage by +0.00% 🎉

Coverage data is based on head (b771fe5) compared to base (6f115b2).
Patch coverage: 100.00% of modified lines in pull request are covered.

Additional details and impacted files
@@           Coverage Diff           @@
##           master     #127   +/-   ##
=======================================
  Coverage   97.96%   97.96%           
=======================================
  Files         135      135           
  Lines       10940    10943    +3     
=======================================
+ Hits        10717    10720    +3     
  Misses        223      223           
Impacted Files Coverage Δ
src/Corrade/Containers/OptionalStl.h 100.00% <100.00%> (ø)

Help us with your feedback. Take ten seconds to tell us how you rate us. Have a feature suggestion? Share it here.

☔ View full report at Codecov.
📢 Do you have feedback about the report comment? Let us know in this issue.

@mosra mosra added this to the 2022.0a milestone Oct 19, 2022
@mosra
Copy link
Owner

mosra commented Oct 19, 2022

Since I'm deep in the reviews already, let's unblock this too, finally (it's from February? wow 😅).

As the change now boils down to #include <utility> to get declarations of std::tuple_element and std::tuple_size, I don't think it's needed to invent any special forward-declarating header for these two -- the size of <utility> is rather small, while maintenance cost of the forward-declaring header and potential for strange breakages after compiler upgrades would be far higher than that. So just drop the TupleStl.h altogether.

What's left is that I assume you'd want to use C++17 auto [a, b] = someType; for as many Corrade/Magnum types as possible, right? In case of Corrade that'd be mainly these I suppose:

  • Containers::Pair, for which the std::tuple_whatever specializations could go into the already-existing Containers/PairStl.h, and a corresponding new C++17-only PairStlCpp17Test.cpp
  • Containers::Triple, for which the std::tuple_whatever specializations could go into the already-existing Containers/TripleStl.h, and a corresponding new C++17-only TripleStlCpp17Test.cpp
  • Containers::StaticArrayView, for which it could go into the already-existing Containers/ArrayViewStl.h, and a corresponding new ArrayViewStlCpp17Test.cpp

For completeness there's also Containers::StaticArray but it doesn't have a corresponding StaticArrayStl.h header yet, that one you'd have to create, and a new StaticArrayStlCpp17Test.cpp as well. (It's also fine if you implement just the Pair and Triple, those are the most important ones.)

It seems like a lot of new test files, but my assumption is that there will be more and more C++14/17/20-specific features (such as your constexpr additions), and the new test files will be reused for those as well.

The std::tuple_element / std::tuple_size is a C++11 feature already so I wouldn't hide them in any #if compiling_as_cpp17. Having them exposed unconditionally would mean std::get<N>() working with Corrade/Magnum types even under C++11, for example.

In case of Magnum I assume you'd want Math::Vector exposed this way, so that'd be a new VectorStl.h, and a corresponding new C++17-only VectorStlCpp17Test.cpp.

@mosra mosra changed the title [WIP] add structured binding forward declaration std::tuple_element / tuple_size specializations for Corrade containers Nov 11, 2022
@sthalik
Copy link
Contributor Author

sthalik commented Nov 11, 2022

The std::tuple_element / std::tuple_size is a C++11 feature already so I wouldn't hide them in any #if compiling_as_cpp17. Having them exposed unconditionally would mean std::get() working with Corrade/Magnum types even under C++11, for example.

For types without an already-existing get<N> member function, this makes the implementation uglier since if constexpr is from C++17 and functions can't be partially specialized.

@sthalik sthalik force-pushed the pr-structured-binding-fwd branch 3 times, most recently from b927aa3 to c6d8f93 Compare November 11, 2022 14:14
@sthalik sthalik force-pushed the pr-structured-binding-fwd branch from c6d8f93 to b771fe5 Compare November 11, 2022 14:21
@sthalik sthalik marked this pull request as ready for review November 11, 2022 16:01
@sthalik sthalik closed this Nov 12, 2022
@sthalik sthalik deleted the pr-structured-binding-fwd branch November 12, 2022 17:51
@mosra
Copy link
Owner

mosra commented Nov 12, 2022

So, not going to do the specializations for other containers either? I'll convert that to an issue, then, in case it eventually becomes useful for someone else.

@sthalik
Copy link
Contributor Author

sthalik commented Nov 12, 2022

I still plan on doing it for Pair and Triple at least, but have more pressing issues at this time.

@mosra mosra changed the title std::tuple_element / tuple_size specializations for Corrade containers [WIP] add structured binding forward declaration Nov 12, 2022
@mosra mosra removed this from the 2022.0a milestone Nov 12, 2022
@mosra mosra added the scrapped label Nov 12, 2022
@mosra mosra added this to the 2023.0a milestone Dec 8, 2023
@mosra
Copy link
Owner

mosra commented Dec 8, 2023

For anyone still subscribed to this PR, a variant of the above together with structured bindings for StridedDimensions was added in 087ead3 and 5f4fc52.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
Development

Successfully merging this pull request may close these issues.

2 participants