Modify pointer to std::string inside C++ class

Question

I just started to learn C++ and encountered an inconsistency using gnu compiler on the one hand and using Visual C++ and Intel compiler on the other hand. Following example defines a class Person with a pointer to a std::string Name. Within method Person::set the string is assigned by value. I'm sure the better way would be to use a pointer, but that's not the question here.

#include <iostream>
#include <string>

class Person
{
   std::string *Name;
public:
   Person(std::string *n);  //Constructor
   void print();
   void set(std::string n);
};

Person::Person(std::string *n) : Name(n) //Implementation of Constructor
{
}

// This method prints data of person
void Person::print()
{
    std::cout << *Name << std::endl;
}


void Person::set(std::string n)
{
    Name = &n;
}

int main()
{
    std::string n("Me");
    std::string n2("You");
    Person Who(&n);

    Who.print();
    Who.set(n2);
    Who.print();


    return 0;
}

The gnu compiler gives following result as I would have expected:

Me
You

But the Visual C++ and Intel compilers result in an undefined behviour. I guess the problem is the life time of the copied variable n in Person::set. Why is it still available after finishing Person::set using gnu compiler and not available using Visual C++ and Intel compilers?

juanchopanza · Accepted Answer · 2014-05-02 09:22:42Z

5

Your Set method is setting you up undefined behaviour because you are taking the address of a local variable, and then use it in another scope:

void Person::set(std::string n)
{
    Name = &n; // n is a local variable
}

Any attempt to de-reference Name outside of Person::set, as you do in Person::print(), is undefined behaviour.

The behaviour of all the compilers you tried is compatible with undefined behaviour, because everything is.

edited May 2, 2014 at 9:22

answered May 2, 2014 at 8:43

juanchopanza

228k35 gold badges421 silver badges493 bronze badges

Sign up to request clarification or add additional context in comments.

4 Comments

juanchopanza Over a year ago

If there is anything incorrect with this answer I would like to know. If there is any other good reason for the down-vote, it would be useful to know too so I can consider fixing it if appropriate.

Holger Over a year ago

Exactly the answer I was looking for. Thank you.

Kerrek SB Over a year ago

n is not a temporary. It's a local variable. It has an address and there's nothing undefined about storing that address.

juanchopanza Over a year ago

@KerrekSB Good point, thanks. I re-phrased the answer. I hope it is clearer now.

Kerrek SB · Accepted Answer · 2014-05-02 08:43:49Z

3

That code is completely broken and unusable. Inside Person::set, n is a local variable, so its address &n is meaningless outside that function. Storing it is pointless, and using to it later is undefined behaviour.

Here's a correct way to write your class in modern C++:

class Person
{
   std::string Name;

public:
   explicit Person(std::string n) : Name(std::move(n)) { }

   void set(std::string n) { Name = std::move(n); }

   void print() const { std::cout << Name << '\n'; }
};

answered May 2, 2014 at 8:43

Kerrek SB

480k96 gold badges904 silver badges1.1k bronze badges

2 Comments

rubenvb Over a year ago

Are the move's really necessary? Won't an optimizing compiler do at least something similar without them?

Kerrek SB Over a year ago

@rubenvb: I don't think anything allows a compiler to ignore the rules of overload resolution, non?

bobobobo · Accepted Answer · 2014-05-02 08:53:58Z

1

The "better way" isn't always to use a pointer in C-like languages. Touché.

Even if you correctly passed a pointer to set:

void Person::set(std::string* n)
{
    Name = n; // Correctly saves
}

You have no idea when n will be deallocated, since it is stored outside the Person class. A better way to do this would be to keep a copy of Name inside the Person class, and pass in new strings by reference:

class Person
{
   std::string Name;
public:
   Person(const std::string &n);  //Constructor
   void print();
   void set(const std::string &n);
};

Person::Person(const std::string &n) : Name(n) //Implementation of Constructor
{
}

void Person::set(const std::string &n)
{
    Name = n; // Person class keeps its own copy of n
}

edited May 2, 2014 at 8:53

answered May 2, 2014 at 8:48

bobobobo

67.9k67 gold badges272 silver badges375 bronze badges

1 Comment

Holger Over a year ago

Thank you, but I was aware that it is stored outside the class. So your answer doesn't really answer my question. I just wanted to know how long n lives inside set method.

Collectives™ on Stack Overflow

Modify pointer to std::string inside C++ class

3 Answers 3

4 Comments

2 Comments

1 Comment

Your Answer

Hot Network Questions

Collectives™ on Stack Overflow

3 Answers 3

4 Comments

2 Comments

1 Comment

Your Answer

Sign up or log in

Post as a guest

Related