3

I am not going to past the whole source because it is 1000+ rows, but I have specially constructed a similar case about the matter I am interested in. Pay attention to this source code:

#include <iostream>
using namespace std;

class Person
{
public:
    Person();
    Person(char*);
    ~Person();
    Person& operator=(const Person&);
    friend Person& example(const Person&);
    void print() const;
private:
    char* name;
};

Person::Person()
{
    name = new char[12];
    name = "Temp";
}

Person::~Person()
{
     delete[] name;
}

Person::Person(char* _name)
{
    name = new char[strlen(_name)+1];
    strcpy_s(name,strlen(_name)+1,_name);
}

Person& example()
{
    char* TestName = new char[11];
    TestName = "ShouldStay";
    Person B(TestName);
    return B;
}

void Person::print() const
{
    cout << name;
}

int main()
{
    example();
    return 0;
}

In this case the example() function will return:

  • example returned {name=0x007cad88 "îþîþîþîþîþîþîþîþîþîþîþîþîþîþîþîþîþîþîþîþîþîþîþîþîþîþîþîþîþîþîþîþîþîþîþîþîþîþîþîþîþîþîþîþîþîþîþîþîþîþîþîþîþîþîþîþîþîþîþîþîþîþîþîþîþîþîþîþîþîþîþîþîþîþîþîþîþîþîþîþîþîþîþîþîþîþîþîþîþîþîþîþîþîþîþîþîþîþîþîþ... } Person &

So obviously the destructor is called on the return command and delete the memory in the heap (so I am not able to do anything further with the pointer - it is pointing to already freed memory - no data).

My question is - How to avoid such behavior? What is the most elegant way to avoid such issue?

Thank you in advance!

mirosoft
  • 285
  • 1
  • 2
  • 7
  • 1
    Return by value. Follow the rule of five or even better, the [rule of zero](http://flamingdangerzone.com/cxx11/2012/08/15/rule-of-zero.html). – dyp Apr 02 '14 at 22:03
  • 1
    `TestName = "ShouldStay";` is not right either. – David G Apr 02 '14 at 22:03
  • Thank you dyp! 0x499602D2 - It is looking OK ... – mirosoft Apr 02 '14 at 22:08
  • 1
    Note that you *need* to define a (custom) copy constructor for this class for return-by-value to work properly. (Or follow the rule of zero ;) – dyp Apr 02 '14 at 22:09
  • `TestName = "ShouldStay";` is both deprecated (won't work with modern compilers) and a memory leak. It's overwriting the *pointer* `TestName` instead of copying the *string* it is pointing to. Use `std::string` to store strings. – dyp Apr 02 '14 at 22:10
  • Yes, dyp, Thank you. I have already implemented copy constructor in the original scenario. Unfortunately, I am dealing with array of objects in the real case and not array of chars (so string workaround is not doable). Furthermore - If I want to implement some more sophisticated operator like operator* which is making some complicated calculations - I hit the same issue too... – mirosoft Apr 02 '14 at 22:16
  • 1
    for arrays of arbitrary types, string "workaround" -> vector "workaround" :) (where "workaround" = safe and simple solution) – dyp Apr 02 '14 at 22:17
  • About the depricating issue - It's working OK on Visual Studio 2012 and no memory leak detected. – mirosoft Apr 02 '14 at 22:18
  • Hahaha, Ok. I thought I can avoid implementing more structures and methods in the source .. – mirosoft Apr 02 '14 at 22:20
  • 1
    A string literal like `"hello world"` is of the type *array of 12 `const` char*, which *could* be converted to a *pointer to char* in earlier versions of C++. This conversion does no longer exist in C++. It might still survive for some time in MSVC++ because of backwards-compatibility, but **DO NOT RELY ON IT**. If you try to change any of the characters of a string literal, hell can break loose. As for the memory leak: It's there. MSVC might not detect it, but you're allocating some memory, then forget its address (by overwriting it with the address of some string literal). – dyp Apr 02 '14 at 22:20
  • Re "C++ How can I construct an object inside a class function, do some operations and return the value?", just **do exactly that**. It's that simple. – Cheers and hth. - Alf Apr 02 '14 at 22:26

3 Answers3

1
  1. Use string rather than char[] to avoid having to use new.
  2. Return Person rather than Person&, as locally-scoped classes are destroyed after they leave scope. Although this will cause a copy to happen depending on compiler settings. This depends of course upon providing a proper copy constructor.
  3. To guarantee avoiding a copy, change the signature of example to:

    void example(Person& person)
    

    And fill in the fields of the inputted person inside the function. The scope of that Person will be bound to the calling scope (or wherever else you constructed it). This method has drawbacks though such as you cannot chain the results together.

aruisdante
  • 8,875
  • 2
  • 30
  • 37
  • If he follows step 1, it won't matter for the example case, but you're correct with a ``char[]`` the default copy constructor will not work. Answer updated to reflect. – aruisdante Apr 02 '14 at 22:08
  • @dyp Also, my step 3 pass-by-reference assignment doesn't require any kind of compiler optimization. It just requires that ``example`` be allowed to access the fields of ``Person``. – aruisdante Apr 02 '14 at 22:13
  • Thank you for this answer! - This was just an example of the real issue and in the real issue the things got nasty :) 1) I am not able to use string-a-like workaround, because in the real example I am dealing with array of objects, not array with chars; 2) I already implemented copy constructor so I will try this method (I just need some time for testing); 3) I am not able to void the function, because I have to implement overload operator +. – mirosoft Apr 02 '14 at 22:13
  • It's ok now, but I'd interpret 2+3 as an advice to *generally* pass-by-reference instead of returning by value. The latter should be the default choice IMHO, since typically return value optimization will be applied which is even cheaper than default-construction plus pass-by-reference; for the remaining cases we have move semantics now. – dyp Apr 02 '14 at 22:14
  • It also definitely depends on what the OP is trying to do with the method. It looks like they're trying to do some kind of chaining, so return-by-value is probably the correct option. Updated answer to reflect. – aruisdante Apr 02 '14 at 22:15
1

Your code contains many logical errors:

Person::Person()
{
    name = new char[12];
    name = "Temp";
}

In the above function you allocate a char array of 12 elements, then you simply forget about it and instead make name pointing to a string literal.

Person::~Person()
{
     delete[] name;
}

whoopps. In case Person was build from a default constructor this would delete a string literal. A no-no in C++.

Person::Person(char* _name)
{
    name = new char[strlen(_name)+1];
    strcpy_s(name,strlen(_name)+1,_name);
}

Not 100% sure what strcpy_s is, but the code in this case allocates an array and seems to copy the string into the array. This seems ok (but just strcpy(name, _name); would have been better for many reasons).

Person& example()
{
    char* TestName = new char[11];
    TestName = "ShouldStay";
    Person B(TestName);
    return B;
}

This code is seriously broken. First of all it's returning by reference a temporary object. A Very Very Bad Idea. It's also allocating an array, and once again just forgetting about it and using a string literal instead.

The most elegant way (actually the ONLY way in my opinion) to get your code working is to first understand how the basics of C++ work. You should start first by reading a good C++ book from cover to cover, and only then you should start coding in C++.

Your 1000 lines of source code are most probably just rubbish. I'm not saying you're dumb, just that you don't know the basics of C++. Take care of them first by reading, not experimenting with a compiler.

You cannot learn C++ by experimenting for two reasons:

  • It's a complicate and sometimes even just downright illogical language because of its history. Guessing is almost always a bad move. No matter how smart you are there's no way you can guess correctly what a committee decided.

  • When you make a mistake there are no runtime error angels to tell you so. Quite often it happens that apparently the program works anyway... until it's run by your teacher, boss or spouse. Guessing C++ rules by writing code and observing what happens is nonsense.

Community
  • 1
  • 1
6502
  • 112,025
  • 15
  • 165
  • 265
  • `strcpy_s` = MS "safe" version of `strcpy` with buffer size "checks" – dyp Apr 02 '14 at 22:23
  • *and only then you should start coding in C++* and what about the exercises in that book? ;) – dyp Apr 02 '14 at 22:24
  • @dyp: in this case `strcpy` would have been better (assuming there was a reason for not using `std::string`, of course). – 6502 Apr 02 '14 at 22:24
  • I also think that the majority of those errors are simply from the OP trying to invent a simplified example that replicates their problem. They pretty explicitly said this isn't the code they're working with. But as you said, that's pretty much a pointless thing to do in C++ unless you really, really know what you're doing because odds are your 'simplified' example will have errors that completely mask the actual problem you were trying to diagnose. – aruisdante Apr 02 '14 at 22:27
  • @aruisdante: the pattern `x = new char[4]; x = "foo";` (repeated twice) clearly shows there's no understanding of what a pointer is and how memory allocation works in C++. My guess is that the code they're working with is just the same horror movie, just much longer. – 6502 Apr 02 '14 at 22:29
  • @6502 I don't disagree at all. Especially since like a ``std::vector`` would probably do what the OP wants in their actual code without using new/pointers. – aruisdante Apr 02 '14 at 22:30
  • Thank you for your constructive answer! I should add that this is just a fast written example of the real issue -> I will quote you "This code is seriously broken. First of all it's returning by reference a temporary object". The code is broken by purpose :) Anyway, Thank you for your fast comment! I believe dyp already resolved the issue I was asking about :) – mirosoft Apr 02 '14 at 22:33
  • @mirosoft: I hope you're not offended because this was not my intention. You've a much bigger problem than being able to return a class instance. You're learning C++ by experimenting, a move that you will regret a lot in the future and that will get on you the curses of the poor souls that will have the misfortune of being in charge of maintaining the horrors you're going to build. – 6502 Apr 02 '14 at 22:38
  • @6502: I am not offended by critics when it is based on a constructive syllogism but still - there are some statements I do not agree. I think experimenting is the fundamental process of learning the world behind / inside us. I pursue everyone to experiment :) Furthermore I am flattered that you define me as somehow professionally bind to C++ but you should not worry - I am maintaining my horrors by myself and I do not gain any material profits from them, neither I use them as commercial products. I am hobbyist. Thank you again and god save the "poor souls"! – mirosoft Apr 02 '14 at 22:55
  • @mirosoft: You can waste your time in any way you like. I agree with you that, in general, one can learn a lot more about programming by writing than by reading. Write write write, and once you're done, rewrite again. However there are exceptions to this rule and sometimes reading first can save a lot of time. Many of the difficulties of C++ are not essential and unknown, but a minefield of shallow traps for which there is a map; reading the map first will save **a lot** of trouble. But like I said before you're of course free of re-making the same big mistake I made in the 90's. – 6502 Apr 03 '14 at 06:07
  • @6502: Thank you for your advice and I should add that I already got some of the books you mention in the answer above and will initiate a thoroughly read (specially Bjarne Stroustrup's ones). Stay tuned about my new questions - I promise their quality and hardness will rise exponentially. :) Thank you again! – mirosoft Apr 03 '14 at 08:33
0
Person& example()
{
    char* TestName = new char[11];
    TestName = "ShouldStay";
    Person B(TestName);
    return B;
}

The above creates an instance of Person on the stack, scoped to the function. So when the function returns, the destructor for Person is called and a reference to the destroyed object (on the stack) is returned.

In addition you should either consider returning a copy of Person, or you need to use the new operator to create an instance of person on the heap and return a pointer to that.

R M
  • 96
  • 1
  • 4