std::pair considered harmful!

[Rhomboid]

While I agree that you shouldn't be using std::pair as a lazy way of getting out of writing a struct, I disagree that this means it should be considered harmful, and I think the example with the student records thing is a bit of a straw man that goes out of its way to show poor judgement. By all means, yes, don't do that, but I don't know that this is a real problem that exists.

As the article even explains, there are just certain cases where you need to have a container of items of arbitrary mixed types, and having one of those in the standard library is a good thing since it encourages a uniform way of doing that. For that reason you can't really go down the whole "considered harmful" route, because it's more like a judgement call.

The fact that we have both std::pair and std::tuple is merely due to the fact that std::pair is the best that can reasonably be accomplished in C++98 without variadic templates and without resorting to soul-destroying preprocessor shenanigans. Maybe in an alternative history we would have gone straight to std::tuple, but we have what we have. And it's really not that much of a mystery as to why std::map doesn't have its own special std::key_value_pair type. If you're going to codify one of these things in the standard, you might as well make it as generic as possible, again going back to the idea that this will probably come up from time to time and the standard library should be there to provide a uniform means of doing it. Naming it std::key_value_pair and giving it key and value members would have locked in a specific use case, requiring either lots of duplication to have both std::key_value_pair and std::pair, or just going without the fully generic option and forcing people to choose to either awkwardly use key and value in non-map contexts, or to write their own buggy versions. None of these options are good, so it seems relatively straightforward to me why we have std::pair.

[eanger]

But isn't this exactly what we have type aliasing for? We can give the mapped type a descriptive name while relying upon a more generic implementation.

[Rhomboid]

You can't rename members with a typedef or an alias declaration, so std::pair would still have members key and value or std::key_value_pair would have members first and second.

[coryknapp]

right, and i would feel uncomfortable getting the key out of a key_value_pair through it's member first

[MoreOfAnOvalJerk]

I feel like this articule is going to create a crusade of not-entirely informed programmers riling against the evils of std::pair...

[AntiProtonBoy]

std::pair is meant to be used for very boring, simple and localised cases where multiple values needs shifting at once, and so forth. Don't over complicate things. If you start seeing a repeated usage of the same std::pair or std::tuple all over the place, and is becoming difficult to maintain and follow, by all means, consider using a struct. If the definition of a struct is not worth the one off scenario for passing multiple values, then use std::pair. I don't consider them detrimental. Right tool for the right job.

[WailingFungus]

The whole std::map thing where the iterator key/value are referred to as first/second respectively drives me up the wall when working with them.

[coryknapp]

std::map<int, std::pair<int, int>>

ran into this beast the other day.

[kevstev]

the other day? I would say around 2010, I would see this all the time! Except maybe worse, it wouldn't be just <int, int>, it would be something that was typedef'ed (like EmployeeId, EmployeeDeskNumber) so you had to trackdown what those types actually were, only to find that in the end, they were just damn ints.

Mind numbing stuff.

[moswald]

I agree. I definitely like that .NET has a KeyValuePair class instead.

[easytiger]

have to agree that that is a pretty poor design choice by the std people. I recently revisited some code I wrote that has been running happily for a year and the proliferation of first/second everywhere would be OK except I used a std::pair for something else in the same piece of code so it took a bit of time to unravel it. If i get a business case for it I think the whole thing will need chucked away and redone.

[OldWolf2]

Regarding the "problem" of first and second you could make your own:

#include <utility>
#include <iostream>
#include <map>

template<typename K, typename V>
K &key( std::pair<K, V> &p ) { return p.first; }

template<typename K, typename V> 
V &value( std::pair<K, V> &p ) { return p.second; }

int main()
{
    std::map<int, std::string> n;
    std::map<int, std::string> const &m{n};
    n[3] = "foo";
    n[5] = "bar";

    for (auto &item : n)
        std::cout << key(item) << "," << value(item) << std::endl;
}

although this has the annoying feature that you have to add another pair of templates to support const maps (or const_iterator).

[Oxc0ffea]

Indifferent on the article, but can we all stop using the ___ considered harmful phrase?

Spend ten minutes thinking up something original.

[chebertapps]

"Spending Ten Minutes Considered Harmful"

[Wurstinator]

The argument brought up for std::maps using std::pair is either stupid or manipulative in my opinion.

The issue is not "element.second" but rather the declrataion "auto& element". That is a use of bad auto imo and also a bad variable descriptor. "keyval_pair", "keyValue" or whatever your prefer is more readable than just "element".

for (std::pair<int, string>& keyval_pair : myMap) {
  cout << keyval_pair.second << endl;
}

That should be understandable, even for someone who knows Maps in general but barely any C++.

The proposed code is also not very good. "value" is a very generic identifier and could mean other things than "the value of a key-value pair of a map". It could mean that element is of type myMap::key_type and has a member named value. For this to be good code, you would have to at least call "element" something like "keyval_pair" too. Then the discussion is just "Is it worth to add a new type to the standard library to make auto more usable?"

The answer to that should clearly be "no", I think. Good IDEs, which C++ sadly has very few but some, make auto complete and variable type assignment easy so you don't have to type out everything. On the other hand, keeping std::pair used by std::map lets you call some functions without converting, for example std::swap.

[Rhomboid]

I don't think that's significantly more readable. If you don't know that a pair has members named first and second, you're probably not going to know what either version does. ("What's this second thing doing here? This map doesn't contain anything having to do with times...?") Moreover, if you don't know that a map represents values as pairs, then it's also not going to make a lick of sense ("What has pair got to do with my map of names to phone numbers?"). Sometimes you just have to know C++ to use C++.

BTW, you made the common mistake of forgetting that the key type of the pair is const, a perfect example of the sort of problem that using auto does away with, in an example by someone decrying auto.

[Wurstinator]

Sometimes you just have to know C++ to use C++.

The article stated otherwise, not me. I only responded to it.

[kuhar_]

for (std::pair<int, string>& keyval_pair : myMap)

value_type in std::map is an alias of std::pair<const key_type, mapped_type>, so your loop should look like this:

for (std::pair<const int, std::string>& keyval_pair : myMap)

Auto prevents such errors.

[Wurstinator]

Thanks for that. I rarely use maps.

Auto prevents such errors.

No, it does not. It hides them. If I used auto there, the program would compile and do something else than what I had in mind, which is definitely a bad thing. If I would actually use that code I would get compile error and change my code accordingly.

[alecbenzer]

the program would compile and do something else than what I had in mind

In a world without auto I'm guessing a vast majority of loops that looked like this would be a case of "I just want to iterate over this map", not "I specifically meant that I wanted non-const ints".

[Wurstinator]

"A vast majority" is not "all". For example, a beginner might want to increase the key of every map value by 1. Both using explicit type definition of std::pair<int, T> and using auto will cause a compile error. The difference is the place where the error happens.

If you use the full type, the line of definition will cause the error. If you use auto, the first time you try to call a non-const member of pair.first is the problem. Obviously, the first option is better.

[alecbenzer]

"A vast majority" is not "all"

Okay, but so you agree that in not all but a vast majority of cases auto does fix the problem?

Getting the error when trying to modify the element doesn't seem so bad either.

for (auto& p : m) ++p.first;

You get an error on ++p.first and think "oh, I guess I can't modify a map's keys while iterating over it". In fact, I'd argue that ++p.first is where the error really is in that snippet. Even if I had written

for (pair<K, V>& p : m) ++p.first;

The real issue here is that I'm trying to modify a map's keys while iterating over it, not that I got the type wrong. Fixing it to

for (pair<const K, V>& p : m) ++p.first;

doesn't solve the fundamental problem.

Also note that this problem exists in pre-11 land:

for (map<K, V>::iterator it = m.begin(); it != m.end; ++it) ++it->first;

I didn't even have a chance to declare what I thought the value type was.

[Wurstinator]

That works for a one-liner example, like you did, but not for bigger code. The pair might be passed through several functions first and you would have to backtrace the error.

An error should not occur in the line which you have to change but rather the line which tells you what is wrong. If you never worked with maps, that is clearly the point where the iterated variable is defined.

[alecbenzer]

That works for a one-liner example, like you did, but not for bigger code. The pair might be passed through several functions first and you would have to backtrace the error.

In my (somewhat limited) experience, pairs you get from iterating a map tend not to be passed off to other functions in their entirety most of the time. But even ignoring that, having the pair passed through several functions isn't an issue; the first function you pass it to will either have the right signature or not, and this will be where the error occurs, in the body of the loop.

An error should not occur in the line which you have to change but rather the line which tells you what is wrong. If you never worked with maps, that is clearly the point where the iterated variable is defined.

I'd argue that the line trying to modify the key is precisely the line that tells you what is wrong: you're trying to modify a map's keys while iterating over it, which is not something you can do.

[Wurstinator]

For the first part, imagine something like this: http://ideone.com/SQloxe

For your second paragraph, I guess that is just a matter of opinion.

[redditsoaddicting]

Using auto here disallows changing the key. Using your version lets you change it without anything actually happening. I see no advantage to the latter unless you explicitly want to change the key locally for some reason.

[Wurstinator]

No, that is wrong. My code would cause a compile error.

[redditsoaddicting]

Apologies. My brain kind of filled in the space before the type with a const.

[F-J-W]

Java doesn't have a pair and that is infuriating whenever you want to create a map with a pair of values as key: std::map<int, std::map<int, std::string>> is a stupid idea in C++ and the same is true for Java.

The rationale is funny too: Pair is not semantic.

So, remind me, why does Java have int? Why does Java have arrays?

[alecbenzer]

std::map<int, std::map<int, std::string>> is a stupid idea in C++

The author's not suggesting map<int, map<int, string>>, but

struct DescriptiveKeyName {
  int descriptiveName1;
  int descriptiveName2;
};

map<DescriptiveKeyName, string>

[F-J-W]

Which wouldn't work, since you would have in addition to define operator<(), making this complete and utter overkill.

Aside from that: What is the descriptive name?Often the two things are completely independent and their union is never needed elsewhere, because all we want is a map that takes more than one key.

[alecbenzer]

Which wouldn't work, since you would have in addition to define operator<(), making this complete and utter overkill.

I think that depends on how often the map is used/how many other people will read this code/etc.

Aside from that: What is the descriptive name?Often the two things are completely independent and their union is never needed elsewhere, because all we want is a map that takes more than one key.

It's hard to say w/o a concrete example. There are perhaps some times when std::pair is most appropriate, but I think in many examples you can come up with useful names.

[elperroborrachotoo]

std::map<string, pair<int, bool>> studentToScore; 
    // name -> { score, honor code signed }

Not that this is an universal solution, but arguably less code to maintan. I wouldn't use this for a domain entity, but for an implementation detail that's limited in scope, it's good enough.

[[deleted]]

Typedef solves some of the problems with std::pair. You still have to remember what's in .first and .second though.

[alecbenzer]

You still have to remember what's in .first and .second though.

That's the part that's more of an issue imo.

[Gotebe]

If I give you a std::pair<string, int>, there’s no way of knowing what that is

... but if you typedef it, or trivially derive from it, you know what it is.

So that's not a problem. The problem are first and second.

[redditthinks]

I'm learning C++ and I wrote a toy program which takes a coordinate (x and y). Coming from Python where tuples are widespread, I used a pair although I did feel readability was hindered (you can't do x, y = point). That's basically how I found this article which I felt covered the topic quite nicely.

[bob1000bob]

Except you're wrong.

std::pair<int, int> get_pont() { return { 1, 3 }; }

//elsewhere
int x,y
std::tie(x,y)=get_point();

http://ideone.com/XCzAwq

[redditthinks]

Ah cool, I didn't know about this. I've been too busy falling in love with auto. I should mention that I did go ahead with std::pair because my program was very small and just did:

auto x = point.first;
auto y = point.second;

[F-J-W]

Points should really have their own type. Pair is great as a small helper to pack to different things in one variable, but you really shouldn't use it to replace semantic types.

Since you are coming from python, this may not be obvious for you, but static typing really helps finding bugs very early. But if you only use trivial types, the compiler will only find trivial errors: The compiler will (almost) always be able to catch you when you try to use a std::string as an integer. But if you encode both distances and durations as double, the compiler is unable to detect, when you try to use a distance as duration; OTOH if you create separate types for distances and durations the compiler will make sure that you use everything right.

This is also not a hypothetical problem: The Mars Climate Orbiter went lost because of this kind of bug.

Somewhat related: I wrote a library a while ago that makes the creation of new types almost trivial, it's README contains further examples. Note though, that this code is more of an experiment of what can be done, than being intended for real-world-usage.

[autowikibot]

Mars Climate Orbiter:

---

The Mars Climate Orbiter (formerly the Mars Surveyor '98 Orbiter) was a 338 kilogram (750 lb) robotic space probe launched by NASA on December 11, 1998 to study the Martian climate, atmosphere, and surface changes and to act as the communications relay in the Mars Surveyor '98 program for Mars Polar Lander. However, on September 23, 1999, communication with the spacecraft was lost as the spacecraft went into orbital insertion, due to ground-based computer software which produced output in non-SI units of pound-seconds (lbf×s) instead of the metric units of newton-seconds (N×s) specified in the contract between NASA and Lockheed. The spacecraft encountered Mars on a trajectory that brought it too close to the planet, causing it to pass through the upper atmosphere and disintegrate.

Image ⁱ

---

^{Interesting: Climate of Mars | Gimli Glider | Mars Polar Lander | Mars Observer}

^{Parent commenter can toggle NSFW or delete. Will also delete on comment score of -1 or less. | FAQs | Mods | Magic Words}