Skip to content
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

F.21 Don't return tuples #2166

Merged
merged 7 commits into from
Jan 25, 2024
Merged

F.21 Don't return tuples #2166

merged 7 commits into from
Jan 25, 2024

Conversation

Eisenwave
Copy link
Contributor

@Eisenwave Eisenwave commented Jan 2, 2024

This PR overhauls and modernizes the current advice. In short, we should not recommend returning pair or tuple.

Rationale

1. Modern C++ has excellent support for struct

C++20 has added operator<=> = default eliminates practically all uses of std::pair. Furthermore, it has made direct-initialization of aggregate types possible, so that structs can be used with emplace, taking away even more reasons to use std::pair.

struct also supports aggregate initialization with designated initializers, unlike std::pair and std::tuple, which aren't aggregates.

2. std::pair and std::tuple are bad for code quality

std::pair hurts code quality. Replacing meaningful member names with first and second is strictly worse. The few cases where there is no meaningful name are usually homogeneous cases (e.g. std::minmax could return std::array<T, 2>).

Consensus in the C++ community has overwhelmingly shifted against the use of std::pair or std::tuple:

  • std::minmax returned std::pair but std::ranges::minmax return std::ranges::min_max_result.
  • std::from_chars returns std::from_chars_result.
  • ...

std::tuple is even worse because it forces the user to access members with std::get<N> which is even more confusing than .first and .second, and tremendously more confusing than .meaningful_name. std::tuple is only useful in variadic templates. This exception is left in the guideline.

3. std::pair and std::tuple have legacy baggage

std::pair is an incredibly bloated type for what it does. It has 11 constructors while providing relatively little value in most of its uses. std::tuple has 28 constructors.

Furthermore, std:tuple is not ABI-trivial in libstdc++, which results in std::tuple<int> being passed via stack and not via register. Overall, a simple struct can be a major boost to compilation speed and even runtime performance.

4. Almost no return type obviously meets the requirement in the old guideline

The old wording states:

The overly-generic pair and tuple should be used only when the value returned represents independent entities rather than an abstraction.

Almost no function obviously meets this hurdle. std::minmax doesn't because it returns a one-dimensional range, specified by a minimum and maximum, not two entirely independent entities. std::from_chars doesn't because it returns a pre-C++23 std::expected-like type. In general, pairs where one member indicates success are abstracted by an optional/expected value.

I find this guideline wishy washy because you can almost always argue that the two entities are not independent, or the other way.

The only example in the standard library I can think of where std::pair clearly makes sense is key/value pairs in `std::map, since the key and value can be accessed and used independently, other than being located in the same data structure. Even so, I'm sure you'll find someone who would argue they are not two independent entities.

5. std::tie is often bad practice

The current guideline contains the positive example:

Sometype iter;                                // default initialize if we haven't already
Someothertype success;                        // used these variables for some other purpose

tie(iter, success) = my_set.insert("Hello");   // normal return value
if (success) do_something_with(iter);

This is bad because

  1. the advice conflicts with C.49: Prefer initialization to assignment in constructors (this is not a constructor, but the same rationale applies)
  2. this advice conflicts with Con.1: By default, make objects immutable (the result could easily be immutable, but late-initialization through std::tie-assignment prevents this)
  3. this advice (at least in this style) conflicts with ES.20: Always initialize an object (assuming Someothertype is meant to be bool-like)
  4. structured bindings have largely obsoleted this practice

std::tie still has a small handful of uses, but most have been replaced by

  • structured bindings (C++17)
  • defaulted comparisons (C++20)

std::tie is more of a historical relic and increasingly niche utility, rather than something we need to recommend in this guideline, especially if what we're recommend is bad practice that conflicts with other rules.

@Eisenwave Eisenwave force-pushed the dont-return-tuple branch 2 times, most recently from 2e154a9 to 21cf8b5 Compare January 2, 2024 13:34
CppCoreGuidelines.md Outdated Show resolved Hide resolved
@hsutter
Copy link
Contributor

hsutter commented Jan 18, 2024

Editors call: Thanks! We're favorable to removing the suggestion to use tuple, but the changes here are more extensive than that. In particular, we would like not to delete the existing Note about Distance example, and not add the new Note about expected. Can this be tightened up to just remove tuple as a return type on the grounds that it is hard to use (has anonymous members etc.)?

@Eisenwave
Copy link
Contributor Author

@hsutter I have mostly kept the old note, but with some modernizing changes now. The old note also mentioned a "variant<T, error_code> type, which I have modernized to optional<T> or expected<T, error_code>, and removed the new note.

Is this a good compromise?

The current build fails because of a spell check in some part I haven't touched.

@hsutter hsutter self-assigned this Jan 22, 2024
@hsutter
Copy link
Contributor

hsutter commented Jan 22, 2024

Interim ack: Thanks! This looks much better. I fixed the spell check issue.

CppCoreGuidelines.md Show resolved Hide resolved
CppCoreGuidelines.md Outdated Show resolved Hide resolved
CppCoreGuidelines.md Outdated Show resolved Hide resolved
@jwakely
Copy link
Contributor

jwakely commented Jan 23, 2024

std::from_chars doesn't because it returns a pre-C++23 std::expected-like type.

As I noted in P2497R0, it's not an expected-like type, because both members have meaning in the error case. It's not a sum type, it's a product type.

The reason it's a named struct not a pair is for the reasons you've given elsewhere in the PR why pair would be a worse choice, but it's not because pair couldn't have been used. The rationale is given in P0067R5.

Copy link
Contributor

@jwakely jwakely left a comment

Choose a reason for hiding this comment

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

Thanks for the updates, LGTM

CppCoreGuidelines.md Outdated Show resolved Hide resolved
@hsutter
Copy link
Contributor

hsutter commented Jan 25, 2024

Thanks!

@hsutter hsutter merged commit 631eccd into isocpp:master Jan 25, 2024
1 check passed
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

4 participants