Passing a pointer of constructor

[seank76]

While going through my company's existing codebase, I saw a bunch of
weird lines.

Take a look at this one:

class A
{
public:
A(Foo *f) : _f(f) {}

private:
Foo *_f;
};

class B : public A
{
public:
B() : A(&(Foo &)Foo())) {}
};

How can you pass "Foo()" as the argument to the constructor of A ?
Wouldn't the memory that is allocated to Foo() get destroyed as soon
as we are out of the scope of B()?

What does this mean?

 

[pmouse]

Interesting...Are you sure this is compilable code?
g++ returns the following error :
error: invalid cast of an rvalue expression of type 'Foo' to type
'Foo&'

 

[Gianni Mariani]

While going through my company's existing codebase, I saw a bunch of
weird lines.

Take a look at this one:

class A
{
public:
A(Foo *f) : _f(f) {}

private:
Foo *_f;
};

class B : public A
{
public:
B() : A(&(Foo &)Foo())) {}
};

How can you pass "Foo()" as the argument to the constructor of A ?
Wouldn't the memory that is allocated to Foo() get destroyed as soon
as we are out of the scope of B()?

What does this mean?

It means you have problems.

The first thing that comes to mind is that &(Foo &)Foo()) might return
the address of a temporary. This means that A(Foo *f) : _f(f) {} is
being initialized to somthing that will be destroyed shortly after the
constructor for A returns.

All around, a very bad thing to be doing.

 

[seank76]

Interesting...Are you sure this is compilable code?
g++ returns the following error :
error: invalid cast of an rvalue expression of type 'Foo' to type
'Foo&'

I can definitely compile this using Sun forte 6.2 compiler.
Did you make sure to define class Foo correctly?

 

[Gianni Mariani]

seank76 wrote:
....

That was my first response when I first saw this code.
However, a colleague of mine has claimed although it is "temporary",
the scope of it belongs to the class instance, NOT the constructor,
hence it is ok.

That would be one weird compiler.

In standard C++ it is not, it's probably not even standard C++.

I don't know if this is right. (The code has been in production for
years so he must be right though...)

or lucky.

I will try to create a test program to validate.

In any case, I just wanted to know if this is a common programming
tactic by anybody.

I have never used it.

 

[seank76]

It means you have problems.

The first thing that comes to mind is that &(Foo &)Foo()) might return
the address of a temporary. This means that A(Foo *f) : _f(f) {} is
being initialized to somthing that will be destroyed shortly after the
constructor for A returns.

All around, a very bad thing to be doing.

That was my first response when I first saw this code.
However, a colleague of mine has claimed although it is "temporary",
the scope of it belongs to the class instance, NOT the constructor,
hence it is ok.
I don't know if this is right. (The code has been in production for
years so he must be right though...)
I will try to create a test program to validate.

In any case, I just wanted to know if this is a common programming
tactic by anybody.

 

[Alf P. Steinbach]

* seank76:

That was my first response when I first saw this code.
However, a colleague of mine has claimed although it is "temporary",
the scope of it belongs to the class instance, NOT the constructor,
hence it is ok.

Bullshit.

 

[seank76]

* seank76:





Bullshit.

--
A: Because it messes up the order in which people normally read text.
Q: Why is it such a bad thing?
A: Top-posting.
Q: What is the most annoying thing on usenet and in e-mail?

Just compiled and ran a test program without any errors using Sun
forte and gcc1.2

the test program is the following:

#include <iostream>
#include <fstream>

using namespace std;

class A
{
public:
A() {}

void doThat() { cout << "HELLOSE SEAN!" << endl; }
};

class B
{
protected:
A* _a;

public:
B(A *a) : _a(a) {}

};

class C : B
{
public:
C() : B((A *)&(const A &)A()) {}

void dodo() { _a->doThat(); }
};

int main()
{
cout << "Starting test." << endl;

C c;

cout << "About the run C.dodo() " << endl;

c.dodo();


return 0;
}

 

[Victor Bazarov]

[..]
Just compiled and ran a test program without any errors using Sun
forte and gcc1.2

the test program is the following:

[..code involving undefined behaviour redacted..]

Go buy a lottery ticket, today is your lucky day.

V

 

[seank76]

[..]
Just compiled and ran a test program without any errors using Sun
forte and gcc1.2

the test program is the following:

[..code involving undefined behaviour redacted..]

Go buy a lottery ticket, today is your lucky day.

V

Wow.

I didn't know there were so many highschool kids in this forum.
Real professionals know they see all kinds of weird **it in the
previous programmers' codes all the time.

But then... I guess this is not really the place to share weird
behaviors of a c++ compiler.

 

[Victor Bazarov]

[..]
Just compiled and ran a test program without any errors using Sun
forte and gcc1.2

the test program is the following:

[..code involving undefined behaviour redacted..]

Go buy a lottery ticket, today is your lucky day.

V

Wow.

I didn't know there were so many highschool kids in this forum.

Finding yourself in familiar company?

Real professionals know they see all kinds of weird **it in the
previous programmers' codes all the time.

That's definitely true. They know. They see. Then they fix it or
throw it away.

But then... I guess this is not really the place to share weird
behaviors of a c++ compiler.

Do share, please, especially if it makes you feel better. None of
weird behaviours matter, but some schoolchildren find it entertaining.
My daughter always responds "I didn't fall!" when given advice not to
climb the furniture that can tip over easily. Well, she's not 4 yet.

V

 

[Alf P. Steinbach]

* seank76:

[quoting signatures]

Please don't quote signatures, please read the FAQ before posting.

 

[seank76]

* seank76:

[quoting signatures]

Please don't quote signatures, please read the FAQ before posting.

Please don't say Bullshit to a topic you don't understand.
Please read "Internet Etiquette" manual before you post.

 

[Alf P. Steinbach]

* seank76:

* seank76:

[quoting signatures]

Please don't quote signatures, please read the FAQ before posting.

Please don't say Bullshit to a topic you don't understand.

Your colleague was bullshitting you.

Both Victor and I are old-timers in this group, both with extensive C++
experience (Victor more than I, I suspect), we're both contributors to
the FAQ, and we're both moderators of the moderated group.

You can generally trust us on technical things. Of course we don't
agree on everything. But those differences are over matters of
interpretation, style and so forth, not over trivial matters of fact.

 

[Old Wolf]

[..code involving undefined behaviour redacted..]

Go buy a lottery ticket, today is your lucky day.

Wow.
I didn't know there were so many highschool kids in this forum.
Real professionals know they see all kinds of weird **it in the
previous programmers' codes all the time.

I think you're missing the point. The code is dreadful and it
is exceptionally (un)lucky that it happens to work. The
object is destroyed as soon as the B constructor finishes.
If you don't fix it then you are the "**it programmer".

In fact the original code is not even well-formed; you fixed
that in your second example though.

Your example only appears to work because on your particular
compiler, it stores that function at a fixed address and that
function did not refer to any of the storage of the A object.
So the function gets through doing what it does, without running
into trouble due to there not actually being any object there.

I stress that this is NOT defined behaviour and is just a quirk
of your system. It might suddenly stop working at any moment.

 

[Gianni Mariani]

* seank76:

[quoting signatures]

Please don't quote signatures, please read the FAQ before posting.

Please don't say Bullshit to a topic you don't understand.
Please read "Internet Etiquette" manual before you post.

Sean, that's a technical term.

 

[James Kanze]

That was my first response when I first saw this code.
However, a colleague of mine has claimed although it is "temporary",
the scope of it belongs to the class instance, NOT the constructor,
hence it is ok.

The collegue is wrong. There are two problems with this code:

-- It binds a temporary to a non-const reference (that is the
effect of the cast). This is illegal, and has been since
something like 1988 or 1989. Still, a lot of compilers
allow it by default. (Sun CC, for example, and I think
VC++, and probably some others.)

-- The lifetime of the temporary ends at the end of the
initialization expression---here, until we return from the
constructor of A. Until the mid-1990's, this was not well
specified, and different compilers behaved differently: g++
destroyed the temporary as soon as it was used (and casting
it counted as a use, so it wouldn't have lasted even into
the call to the constructor of A), CFront based compilers
destructed it at the end of the block (and I don't know when
in this case). Sun CC (which you mention elsewhere that you
use) was originally based on CFront, and even the most
recent versions of the compiler implement the extended
lifetime by default; if you want standard compliance, you
should be specifying something like "-features=tmplife".
(If I were you, I'd check out the -features option for the
version of the compiler you are using. The compiler is very
configurable, and like every other compiler I've used, the
defaults don't correspond to anything I'd want to use.)

I don't know if this is right. (The code has been in
production for years so he must be right though...)

Just because some code happens to work with a particular
compiler doesn't mean it's correct. Especially if that compiler
happens to be designed to support older, non-standard code by
default.

I will try to create a test program to validate.

In any case, I just wanted to know if this is a common programming
tactic by anybody.

I've never seen it before. It is, quite frankly, horrible, and
not conform with modern C++ (where modern means anything since
almost 20 years ago).

 

[James Kanze]

Just compiled and ran a test program without any errors using Sun
forte and gcc1.2

Is that g++ 1.2? g++ is currently at 4.1.something, or more.
Any g++ before about 2.6 or 2.7 is totally worthless. Any g++
before 3.0 is hopelessly out of date.

the test program is the following:

#include <iostream>
#include <fstream>

using namespace std;

class A
{
public:
A() {}

void doThat() { cout << "HELLOSE SEAN!" << endl; }

};

class B
{
protected:
A* _a;

public:
B(A *a) : _a(a) {}

class C : B
{
public:
C() : B((A *)&(const A &)A()) {}

You added a const. That makes the code syntactically correct,
and means that the compiler is not required to issue a
diagnostic.

void dodo() { _a->doThat(); }

int main()
{
cout << "Starting test." << endl;
C c;
cout << "About the run C.dodo() " << endl;
c.dodo();
return 0;
}

The code contains undefined behavior, which means that it might
seem to work. Especially in simple cases. Try adding tracing
calls to the constructors (including the copy constructor, just
in case) and the destructor of A:

class A
{
public:
A() { std::cout << "A::ctor" << std::endl ; }
A( A const& ) { std::cout << "A::copy" << std::endl ; }
~A() { std::cout << "A::dtor" << std::endl ; }
// ...
} ;

With an up-to-date, conformant compiler, you should see the
message from the destructor before the message from A::doThat().

Another thing you might try is to add a member variable to A,
initialize it in the constructor, overwrite it in the
destructor, and output its value in A::doThat(); you should find
that it does not have the correct value. (The reason your code
seems to work, of course, is because you don't actually use
anything in A in doThat(). Just making doThat() virtual might
be enough to cause problems.)

 

[James Kanze]

* seank76:

[quoting signatures]

Please don't quote signatures, please read the FAQ before posting.

Please don't say Bullshit to a topic you don't understand.

What your collegue claimed about the program was bullshit. It
shows simply that he doesn't really know C++.

You asked about particular code. We're telling you. It's not
legal. It never has been. And although it may work with some
older compilers, it would never pass code review in any well run
shop.

If it works, today, it is purely by chance, because you have a
compiler which is out of date, or because the undefined behavior
doesn't happen to reveal the error.

Please read "Internet Etiquette" manual before you post.

He did, I think. At least, it does say to not quote signatures.

 

[seank76]

[..code involving undefined behaviour redacted..]
Go buy a lottery ticket, today is your lucky day.

Wow.
I didn't know there were so many highschool kids in this forum.
Real professionals know they see all kinds of weird **it in the
previous programmers' codes all the time.

I think you're missing the point. The code is dreadful and it
is exceptionally (un)lucky that it happens to work. The
object is destroyed as soon as the B constructor finishes.
If you don't fix it then you are the "**it programmer".

In fact the original code is not even well-formed; you fixed
that in your second example though.

Your example only appears to work because on your particular
compiler, it stores that function at a fixed address and that
function did not refer to any of the storage of the A object.
So the function gets through doing what it does, without running
into trouble due to there not actually being any object there.

I stress that this is NOT defined behaviour and is just a quirk
of your system. It might suddenly stop working at any moment.

Thanks for the answer.
This is what I was looking for, not some little kids telling me it's
bad code.
I am the one who is telling people around me it's bad code.
I just need to convince them why it's bad.

thanks for your help,
Sean.