Sign in to follow this  

Stupid C++ Question

This topic is 2790 days old which is more than the 365 day threshold we allow for new replies. Please post a new topic.

If you intended to correct an error in the post then please contact us.

Recommended Posts

I define a class called XYZ that has an overloaded operator * that takes in a scalar as argument. XYZ XYZ::operator*(float s) { ... } this works for: XYZ xyz1, xyz2; xyz1 = xyz2 * s; How can I make it work for: xyz1 = s * xyz2 ??? What I've learned so far that I cannot. Correct me if I'm wrong. Thanks.

Share this post


Link to post
Share on other sites
You have to make operator* a free (global) function, which takes two arguments (the left and right sides of the *). e.g.
XYZ operator*(float lhs, const XYZ& rhs) { return rhs * lhs; }
Also, your existing operator* should be a const member function:
XYZ XYZ::operator*(float s) const { ... }

Share this post


Link to post
Share on other sites
Quote:
Original post by Sambori
I define a class called XYZ that has an overloaded operator * that takes in a scalar as argument.

XYZ XYZ::operator*(float s) { ... }

this works for:

XYZ xyz1, xyz2;

xyz1 = xyz2 * s;

How can I make it work for:

xyz1 = s * xyz2

???

What I've learned so far that I cannot. Correct me if I'm wrong.

Thanks.


XYZ operator*(float s, XYZ const& b) {
XYZ tmp = b;
return tmp * s;
}



Ideally though you shouldn't define operator* as a member function at all, but instead operator *=, then you can simply rewrite both functions as:

XYZ operator*(float s, XYZ const& b) {
XYZ tmp = b;
return tmp *= s;
}
XYZ operator*(XYZ const& a, float s) {
XYZ tmp = a;
return tmp *= s;
}

Share this post


Link to post
Share on other sites
Quote:
Original post by Washu
snip, snip


Your implementations are a bit inefficient. In the first one, you're already provided a const XYZ, no reason to make a temporary and operate on that. We can make XYZ::operator* a const function, and in operator*(float, const XYZ&) use the const object as-is.

Similarly, if performance is a concern you shouldn't implement both versions using operator*=, since they both create another temporary just to use operator*=.

Explicitly writing operator* functions to avoid unnecessary constructor calls are the way to go here :)

Share this post


Link to post
Share on other sites
Quote:
Original post by Grafalgar
Quote:
Original post by Washu
snip, snip


Your implementations are a bit inefficient. In the first one, you're already provided a const XYZ, no reason to make a temporary and operate on that. We can make XYZ::operator* a const function, and in operator*(float, const XYZ&) use the const object as-is.

You might think they are, but they aren't. The compiler has the ability to elide construction of temporary objects, and even deal with NRVO (any modern compiler can do this). As such premature optimizations like code duplications are not only unnecessary, but frequently can introduce bugs that require changing multiple identical pieces of code to fix.

Share this post


Link to post
Share on other sites
Quote:
Original post by Washu
You might think they are, but they aren't. The compiler has the ability to elide construction of temporary objects, and even deal with NRVO (any modern compiler can do this). As such premature optimizations like code duplications are not only unnecessary, but frequently can introduce bugs that require changing multiple identical pieces of code to fix.


I quickly ran a couple of tests:


#include "stdafx.h"

#define USE_TEMP 1
#define USE_ASSIGN_MUL 1

class Foo
{
public:
Foo()
{
printf("Foo() called\n");
}

Foo(const Foo& src)
{
printf("Foo(copy) called\n");
}

#if !USE_ASSIGN_MUL
Foo operator*(float s) const
{
return *this;
}
#endif

Foo& operator*=(float s)
{
return *this;
}
};

#if !USE_ASSIGN_MUL
Foo operator*(float s, const Foo& foo)
{
#if USE_TEMP
Foo tmp = foo;
return tmp * s;
#else
return foo * s;
#endif
}
#else
Foo operator*(float s, const Foo& foo)
{
Foo tmp = foo;
return tmp *= s;
}

Foo operator*(const Foo& foo, float s)
{
Foo tmp = foo;
return tmp *= s;
}
#endif

int _tmain(int argc, _TCHAR* argv[])
{
Foo a;

printf("\na * 1.0f:\n");
a * 1.0f;

printf("\n1.0f * a:\n");
1.0f * a;

return 0;
}



Visual Studio, 2008, default Win32 Console program, built for Release.

Here's my output, with the defines set:


#define USE_TEMP 0
#define USE_ASSIGN_MUL 0

Output:

Foo() called

a * 1.0f:
Foo(copy) called

1.0f * a:
Foo(copy) called




#define USE_TEMP 1
#define USE_ASSIGN_MUL 0

Output:

Foo() called

a * 1.0f:
Foo(copy) called

1.0f * a:
Foo(copy) called
Foo(copy) called




#define USE_TEMP 1
#define USE_ASSIGN_MUL 1

Foo() called

a * 1.0f:
Foo(copy) called
Foo(copy) called

1.0f * a:
Foo(copy) called
Foo(copy) called



Your implementation, using *=, calls the copy constructor twice, one for each of the temporary objects it needs to use operator *= on. This makes sense to me, I do not see how the compiler can optimize out something that modifies an object, as is the case with operator*=.

Even having a temporary in operator*(float, const Foo&) resulted in an extra copy constructor call. Now that temporary could arguably be optimized out, but it gets tricky when you deal with heavy objects with potential side effects.

Share this post


Link to post
Share on other sites
Quote:
Original post by Grafalgar

calls the copy constructor twice

printf is not side-effect free, so compiler is not allowed to optimize it out.

Remove printf from constructors and examine assembly.

Share this post


Link to post
Share on other sites
*sigh* just because the copy constructor can be elided doesn't mean that it will be. If the copy constructor is non-trivial (as in if you perform a complex operation such as a function call that cannot be trivially inlined), then the compiler will not be able to elide the copy construction.

Perhaps you should try a more realistic example, such as that of a vector (which is what one would assume he's attempting to implement).

Share this post


Link to post
Share on other sites
Quote:
Original post by Antheus
Quote:
Original post by Grafalgar

calls the copy constructor twice

printf is not side-effect free, so compiler is not allowed to optimize it out.

Remove printf from constructors and examine assembly.


Ah, and therein lies my point. We don't know the specifics of the XYZ object that the OP referenced, but if it's a heavy object with side-effects when the copy-constructor is called, then having those temporaries can be quite troublesome.

This is not a premature optimization, this is knowing the consequences of code and the subtleties of C++. I understand the need to have generic, re-usable code, but in this case I consider that over-engineering at the potential cost of performance.

Quote:
Original post by Washu
*sigh* just because the copy constructor can be elided doesn't mean that it will be. If the copy constructor is non-trivial (as in if you perform a complex operation such as a function call that cannot be trivially inlined), then the compiler will not be able to elide the copy construction.

Perhaps you should try a more realistic example, such as that of a vector (which is what one would assume he's attempting to implement).


No need to get snippy. We're having a discussion, are we not? Anyway, that's exactly right: "does not mean that it will be." Your suggestion went to someone that seems new to the language, and you provided him with an implementation that could very well have a non-trivial performance impact. If he is, in fact, implementing a vector then that scalar mul could be called hundreds, if not thousands, of times a frame, and could produce a significant hit on the frametime.

As I said already, your solution, while software-engineering friendly, is an over-engineered one that could come with a non-trivial performance impact.

Share this post


Link to post
Share on other sites
Quote:
Original post by Grafalgar
Ah, and therein lies my point. We don't know the specifics of the XYZ object that the OP referenced, but if it's a heavy object with side-effects when the copy-constructor is called, then having those temporaries can be quite troublesome.


So, let's see if I've got this straight. Your argument appears to be:

"Well, what if it's expensive to copy-construct XYZ and then invoke its operator*=, but cheap to default-construct XYZ and then perform the multiplication manually (which presumably involves overwriting stuff that was set in the default constructor)?"

After all, a heavy object is a heavy object regardless, and operator* requires another object to be created regardless (that's the general contract of multiplication: it produces another value rather than modifying existing ones).

I can only think of one case offhand where this could possibly reflect reality, and in that case operator*= is actually pretty much impossible to implement directly: i.e. for matrix*matrix multiplication. In that case you would of course implement operator* to multiply values into a new matrix, and then implement operator*= to copy from there back into the original matrix.

However, that clearly isn't the case here since we're implementing multiplication by a scalar (float, to be specific).

Quote:
This is not a premature optimization, this is knowing the consequences of code


In light of the above, I am unconvinced.

Quote:
and the subtleties of C++. I understand the need to have generic, re-usable code, but in this case I consider that over-engineering at the potential cost of performance.


I think you have a bizarre concept of "over-engineering" if it includes implementing functions as one-liners in terms of existing functionality that you have to implement anyway.

The point here isn't about making code re-usable; it's about making code re-used, which is ultimately more important.

Quote:
No need to get snippy. We're having a discussion, are we not?


Any "tone" you perceive coming from Washu is the result of literally years (if not decades) of experience dealing with people who think they know more about optimization than they actually do.

Quote:
Anyway, that's exactly right: "does not mean that it will be." Your suggestion went to someone that seems new to the language, and you provided him with an implementation that could very well have a non-trivial performance impact.


Your suggestion, meanwhile, is aimed at convincing someone new to the language to think about a performance impact that dubiously even exists (see above) and is unlikely to matter anyway. It is trivially understood that people new to a language are not going to be using it to do advanced things; performance is therefore not high on the list of priorities. "Make it work, then make it right, then make it fast" has been the well-understood software engineering wisdom since pretty much the beginning.

In any case, the expert C++ programmer initially expects the compiler to perform any straightforward optimizations, and later verifies this if there is reason to be suspicious. If you don't trust the compiler to do its job even that much, you should be writing in assembly instead.

Quote:
If he is, in fact, implementing a vector then that scalar mul could be called hundreds, if not thousands, of times a frame, and could produce a significant hit on the frametime.


And the time to worry about that is if and when it happens.

Quote:
As I said already, your solution, while software-engineering friendly, is an over-engineered one that could come with a non-trivial performance impact.


Repeating yourself does not strengthen your argument. We're not writing high school essays here; there's no need for a redundant "conclusion" paragraph.

Share this post


Link to post
Share on other sites
Zahlman, I was talking about creating temporaries, and unnecessary constructor calls, which could have a performance impact. This isn't something like a replacing bubble-sort with quick-sort over a ten item array called once a second. This is a situation where you can tell that the copy-constructor is called unnecessary, and that the compiler may not optimize it out. It's a red flag that comes up when trying to write efficient code from the start.

It's certainly up to the coder to decide whether or not this is reasonable or not, and so I'm not implying that it's a must-be rule to follow. But if presented with code like this I will grow immediately suspicious of the performance impact. It's akin to calling a function inside a for loop's evaluator (ex, for(int i = 0; i < list.getCount(); i++)... ), in that it's a simple situation to avoid when coding in order to promote efficient code. In this case, why not just store the value of getCount outside the loop and use that variable? Would you raise the premature optimization? If you do, would you suggest a junior programmer do stuff like that and deal with optimizations later, perhaps at the very tail end of the project when you're all red-eyed from staying up too long? "Simple" things to avoid problems later can help you focus on real issues, can it not?

The point is, relying on the compiler to "do its job" is no reason to take great liberties on code. There are countless little things to keep in mind to just write efficient (we're not even at "optimized" yet) code, and considering the construction of temporaries is one of them.

Btw, I could really give two figs about "the countless years of experience" any one person has writing code. If I had a penny for every 'industry vet' with over 'twenty to thirty years' of coding experience that wrote illegible, sloppy, slow, unmaintainable code but then demanded everyone listen to him/her because of said experience.. oi.

Granted, I'm not saying Washu is one them, I'm saying I make no assumptions about a programmer's skill until I see what they can and have done.

Going back to my post in response to Washu where he said that the compiler is smart enough to optimize the constructor calls, I simply showed a situation where it does not, and that it calls for caution when dealing with temporaries. Hell, even Effective C++ spends an entire section on the topic of constructor calls and temporaries.

Quote:
Original post by ZahlmanAfter all, a heavy object is a heavy object regardless, and operator* requires another object to be created regardless


Yes, it does, but Washu's implementation(s) created *two* temporaries when dealing with a constructor that has side-effects that cannot be compiled out.

[Edited by - Grafalgar on April 28, 2010 2:16:26 AM]

Share this post


Link to post
Share on other sites
Quote:
Original post by Grafalgar
Zahlman, I was talking about creating temporaries, and unnecessary constructor calls, which could have a performance impact.

But which often don't have any performance impact, and which sometimes even increase performance as for less deep dependency chains.

E.g., imagine the following is your decoded assembly, then

int x = a * a;
int y = a * a * a;


is in these times probably superiour to

int x = a * a;
int y = x * a;
.


Quote:
This isn't something like a replacing bubble-sort with quick-sort over a ten item array called once a second.

Right. Algorithmic optimizations should be preferred over micro-optimizations.

Quote:
This is a situation where you can tell that the copy-constructor is called unnecessary, and that the compiler may not optimize it out.

That is something you can only tell by looking at the assembly or maybe object code. For thin, traceable, inlineable (constexprable, yay) code you shouldn't care for 99% of time.

Quote:
It's a red flag that comes up when trying to write efficient code from the start.

And it's a red flag for clean, re-usable, maintainable code to apply micro-optimizations and doing redundant things.


Quote:
In this case, why not just store the value of getCount outside the loop and use that variable?

In case of traceable code, the compiler will do it for you (loop invariant code motion). But I have nothing against declaring once (by runtime codepath), and re-using multiple times.


Quote:
Would you raise the premature optimization?

Yes.

Quote:
If you do, would you suggest a junior programmer do stuff like that and deal with optimizations later, perhaps at the very tail end of the project when you're all red-eyed from staying up too long?

Yes.

Once the code is running, you can profile, and find the places where to spend some more money.

Quote:
"Simple" things to avoid problems later can help you focus on real issues, can it not?

Indeed, simple code helps to avoid a plethora of problems in the future.


Quote:
Btw, I could really give two figs about "the countless years of experience" any one person has writing code. If I had a penny for every 'industry vet' with over 'twenty to thirty years' of coding experience that wrote illegible, sloppy, slow, unmaintainable code but then demanded everyone listen to him/her because of said experience.. oi.

About unmaintanability and micro-optimizations...


Quote:
I simply showed a situation where it does not

You have only mentioned "release-mode", but

  • have you even optimizations turned on?
  • do you know that VS2008 is outdated and lightyears behind e.g. gcc 4.5 or icc?
  • is your code really full of printf's and other side effects inside constructors?


[Edited by - phresnel on April 28, 2010 3:14:49 AM]

Share this post


Link to post
Share on other sites
Quote:
Original post by phresnel
But which often don't have any performance impact, and which sometimes even increase performance as for less deep dependency chains.


Granted, but if considering the "a * a * a" line and when using an implementation that uses temporaries (as Washu's initial suggestion), that implementation could have more of impact than if those temporaries are simply avoided. This isn't a micro-optimization, this is just plain efficient coding while you write it.

Quote:

That is something you can only tell by looking at the assembly or maybe object code. For thin, traceable, inlineable (constexprable, yay) code you shouldn't care for 99% of time.


Fair, but there's certainly worth in avoiding it if it literally takes you two seconds. Think of it as more like a habit. A habit that says don't use temporaries if there's no need to. Reduces code, avoids potential performance issues, yadda yadda.

Quote:
In case of traceable code, the compiler will do it for you (loop invariant code motion). But I have nothing against declaring once (by runtime codepath), and re-using multiple times.


I've had many a situation where the compiler did *not* do that, especially when a particularly poor implementation of a count() did not simply return a cached value, but instead recalculated every time that function was called. So, as a result, I am in the habit of *not* calling functions in the for-loop's conditional. I do not consider that a micro-optimization as much as a good habit to avoid issues.

Quote:

Quote:
Would you raise the premature optimization?

Yes.
Quote:
If you do, would you suggest a junior programmer do stuff like that and deal with optimizations later, perhaps at the very tail end of the project when you're all red-eyed from staying up too long?

Yes.


That's utterly terrifying. I've had to deal with this *exact* situation more times than I care to admit. I've sat up literally until 4am to track down performance hits only to discover some programmer, in all his wisdom, called a function in a for-loop, which in turn did a non-trivial amount of processing/counting/whatever. Massive time sink, all because he didn't bother coding for efficiency as he wrote the code. This is also why I am now a contractor, because if I'm going to maintain other people's code until 4am I'm damn-well going to get paid for it :)

(and in all fairness, in many cases that programmer was me. Lessons learned:))

Quote:
Indeed, simple code helps to avoid a plethora of problems in the future.


Hmm. Perhaps this is the misunderstanding here. Let clear things up: I'm not against software engineering principles, good architecture, re-usable code. Quite the opposite, in fact. But I've built and maintained enough code to realize that even 'micro-software-engineering' (heh) can have severe drawbacks. Using "*=" to implement * would, imho, be one of them. It has the potential to come with a cost, and it's up to the programmer to determine if that's worth it or not.

I personally don't care to over-engineer small bits of code. Wastes too much time and, inevitably, doesn't matter in the grand scheme of things. I'd rather spend time on the "big architecture" - the things that, well after the fact, cannot be changed unless you're willing to accept a massive time/money cost.

Quote:
About unmaintanability and micro-optimizations...


See above, but if we talk directly about the implementation of *= and * as stated, that's certainly not 'unmaintainable' by any stretch of the imagination. Here I would argue that spending time to "micro-engineer" the code is to a greater detriment.

Quote:
Quote:
I simply showed a situation where it does not

You have only mentioned "release-mode", but

  • have you even optimizations turned on?
  • do you know that VS2008 is outdated and lightyears behind e.g. gcc 4.5 or icc?
  • is your code really full of printf's and other side effects inside constructors?


Ok, that's very fair, but I fail to see why not get into the habit of coding for efficiency from the start, avoiding headaches at the tail end. Things like avoiding unnecessary temporaries, not calling functions in for loops if it can be avoided, etc. As I said before, I'm not suggesting to write some code, profile it, and get it to run as fast as possible before moving on. Just to get into habits that will allow the programmer to avoid classic pitfalls if at all possible.

In case there's confusion, I make a clear separation of coding-for-efficiency and optimization. The former is a while-you-do-it, employing good habits, etc. It should come automatic. Optimization is sitting down and trying to improve the speed of your code after the fact. The whole point of me concentrating on temporaries, for-loops, etc is to encourage the former *while* you're coding.

Share this post


Link to post
Share on other sites

This topic is 2790 days old which is more than the 365 day threshold we allow for new replies. Please post a new topic.

If you intended to correct an error in the post then please contact us.

Create an account or sign in to comment

You need to be a member in order to leave a comment

Create an account

Sign up for a new account in our community. It's easy!

Register a new account

Sign in

Already have an account? Sign in here.

Sign In Now

Sign in to follow this