C++ Overloading Question

Recommended Posts

Hi guys. I recently studied C / C++ Coding Standards book.

//Function overloading must be avoided in most cases
//Use:
const Anim* GetAnimByIndex(const int index) const;
const Anim* GetAnimByName(const char* name) const;
//Instead of:
const Anim* GetAnim(const int index) const;
const Anim* GetAnim(const char* name) const;

I found this, and I'm really curious why function overloading must be avoided in most cases.

I want to hear exact answer about it. Thx.

Share this post


Link to post
Share on other sites
1 hour ago, Zao said:

In this particular case, the call site may be a bit unexpected if you use NULL or 0 as a null pointer constant, as the integer overload will be chosen.

If you want to avoid this, you need to use "nullptr" instead of avoiding overloads. This, however, depends on the language standard you use which for most books is unfortunately still below C++11/14.

Share this post


Link to post
Share on other sites
Posted (edited)

The only time overloading becomes very tricky and problematic is in the case of universal reference templates.

template< typename T > 

void Func(T &&arg);

This becomes even more tricky with variadic template arguments.

template< typename... T >

void Func(T&&... args);

Which becomes even more tricky with constructors. ;)

In all other cases, overloading shouldn't be a big deal.

Edited by matt77hias

Share this post


Link to post
Share on other sites

This C++ coding style guideline is NOT about readability.  Not if you mean making people THINK they like reading the code.  This is about true readability, accuracy, debug-ability.  The overload based version reads easier ... you don't think about the function as 2 functions, and you don't have to think about the type.  However, these methods are actually 2 different lookups, semantically, so they should have 2 different names - If I converted the int 5 to the string "5" and called the method, it would NOT return the animation with the index 5, but instead of return a null pointer because there is no animation with the NAME "5".  As C++ has evolved with all sorts of anonymous and auto typing, this kind of thing would be almost 100% invisible to a reader and even tough for a debugger.

There is nothing wrong with overloads like in the math library:

float sqrt(float)

double sqrt(double)

because they are semantically identical.  Never mix (basic) TYPE selection with semantic selection.

There are reasons to violate every "rule".  But in general only violate rules like this in methods are only FOR violating this rule.  in other words a method with overloads that change the behavior based on the type of the input, should ONLY be a facade for selecting different behaviors based on type 

Share this post


Link to post
Share on other sites
Posted (edited)
On 8/27/2017 at 5:49 AM, Hodgman said:

Personally I believe you should not write code where using the slow path and using the fast path will look the same. It should be obvious when reading the calling code whether they're doing something dumb (repeatedly doing unnecessary string comparisons) or not.

While I agree with the idea of "you should not write code where using the slow path and using the fast path will look the same", I'm not sure it applies that much here.  The two functions still take two different arguments, one an index and one a string.  So looking at the code it should be clear that you're calling the one that takes the string, and hence would be expected to be slower.  Adding "ByName" to the end of the function only mostly succeeds in making the function name longer and more redundant, since the input type already tells you the same information.

Edited by 0r0d

Share this post


Link to post
Share on other sites
1 hour ago, 0r0d said:

Adding "ByName" to the end of the function only mostly succeeds in making the function name longer and more redundant, since the input type already tells you the same information.

I guess it depends what your naming convention for variables is. If you're using some horrible systems hungarian where you attach the data type to the variable name, then sure, you'd have something redundant like:

GetAnimByName( pszMyAnimation1 ); // slow string-based logic
GetAnimByIndex( uidxMyAnimation2 ); // fast index-based logic

But if your variable is just called "myAnimationN", then the call-site could look like:

GetAnim( myAnimation );//is this the fast path or the slow path?

 

Share this post


Link to post
Share on other sites
1 hour ago, Hodgman said:

I guess it depends what your naming convention for variables is. If you're using some horrible systems hungarian where you attach the data type to the variable name, then sure, you'd have something redundant like:


GetAnimByName( pszMyAnimation1 ); // slow string-based logic
GetAnimByIndex( uidxMyAnimation2 ); // fast index-based logic

But if your variable is just called "myAnimationN", then the call-site could look like:


GetAnim( myAnimation );//is this the fast path or the slow path?

 

You will almost always have context around that line of code to tell you what the variable is, even if the variable isnt named in a descriptive manner.   The problem with your example isnt the "GetAnim()" part, it's the fact that the variable is named "myAnimation".  Looking at "myAnimation" I'd expect I'm looking at an instance of an Animation object, or perhaps a reference (or pointer) to one.  I would not expect to be looking at either an animation name, or animation path, or animation index.  But then again I like to give variables appropriate names.

Again, it seems that the blame is being put on the wrong things here.  Overriding GetAnim() to take an index and a name seems like a perfectly sensible thing to do.   It's a descriptive and unambiguous function name.  Saying that it's problematic because the programmer gives variables ambiguous names seems to totally miss the real issue.   

 

Share this post


Link to post
Share on other sites
13 minutes ago, 0r0d said:

Again, it seems that the blame is being put on the wrong things here.  Overriding GetAnim() to take an index and a name seems like a perfectly sensible thing to do.   It's a descriptive and unambiguous function name.  Saying that it's problematic because the programmer gives variables ambiguous names seems to totally miss the real issue.   

I'm pretty sure this is a subjective / style choice, not an objective truth ;)

Ambiguous function names vs ambiguous variable names. Take your pick! If one seems more sensible, that's a stylistic choice, hopefully with some reasoning, but also with some gut feeling. 

My argument as to the function name being ambiguous is that, from a performance point of view, they're extremely different functions. One is a very expensive *search* operation on variable length keys, the other is a zero-cost array indexing operation. Very different things - misleading and outright dangerous to pretend they're at all similar in any way.

A high level gameplay programmer might also argue that they're semantically identical - no difference at all! They're both just key/value lookups for an object with two keys. Of course they should be named the same.

You can understand an argument without having to agree with it :)

If this was deep inside the engine, I would give more weight to the first argument. If this was being bound to Lua scripted gameplay code, I would give more weight to the second argument. Sometimes code styles are only appropriate within certain problem domains.

Share this post


Link to post
Share on other sites
8 minutes ago, Hodgman said:

 

My argument as to the function name being ambiguous is that, from a performance point of view, they're extremely different functions. One is a very expensive *search* operation on variable length keys, the other is a zero-cost array indexing operation. Very different things - misleading and outright dangerous to pretend they're at all similar in any way.

Are you saying the two functions are not similar in any way??

Ok, so you're arguing that the functions are ambiguous, but like I said it's usually the case that you know exactly what's being passed into that function.  Maybe not in something like LUA, but in typical c++ code you will know.  (one reason why I try to keep the use of auto to a minimum)   And you will certainly know if you're writing the code to begin with, since you must have looked at the header and seen the function prototypes.

In any case, you're arguing a specific case and the OP was asking a general question.  

"why function overloading must be avoided in most cases"

In most cases, if you have functions that do the same thing and are only different in what single parameter they take in, I'd say that naming them the same is a perfectly valid thing to do.  Yes, it's a matter or preference, but really only that.

Share this post


Link to post
Share on other sites
Posted (edited)

I'm against overloading.

Sticking with a single fundamental access type and using external converters leaves the class definition untouched/stable.

const anim* get_anim( const int &index ) const;

get_anim( std::stoi( "4" ) );

std::map< std::string, int > anim_name;

get_anim( anim_name[ "wobble" ] );
  
// maybe...
get_anim( button.get_index() );

 

Edited by m3mb3rsh1p

Share this post


Link to post
Share on other sites
Posted (edited)
// returns the animation with the given index, or nullptr if the index is invalid
const Anim* TryAndGetAnim(const int index) const;

// returns the index associated with the name, or -1 if it fails to find it
int FindAnimIndex(const char* name) const;

Now you can get the same functionality, but the function isn't repeated twice. No overloading necessary. The "finding index" and "getting the animation" concepts are now in separate functions and can be used in other places as separate things.

I am not opposed to overloading, but I don't think this is the best use case for it.

Edited by Oberon_Command

Share this post


Link to post
Share on other sites
14 minutes ago, Oberon_Command said:

// returns the animation with the given index, or nullptr if the index is invalid
const Anim* TryAndGetAnim(const int index) const;

// returns the index associated with the name, or -1 if it fails to find it
int FindAnimIndex(const char* name) const;

Now you can get the same functionality, but the function isn't repeated twice. No overloading necessary. The "finding index" and "getting the animation" concepts are now in separate functions and can be used in other places as separate things.

I am not opposed to overloading, but I don't think this is the best use case for it.

I find the "Try" a bit ambiguous since the name is also associated with try/catch clauses. "GetAnim" should suffice, for the remainder (i.e. what happens if the index does not match anything?), you should read the specifications of the method, IMHO.

Furthermore, it could be beneficial to have multiple ways of accessing the "Anim" with regard of performance (I am not saying they should all have the same name). Of course: make it work -> make it right (refactoring) -> make it fast (profiling/optimization); so alternative ways (if beneficial) should only be added in the end.

Share this post


Link to post
Share on other sites
Posted (edited)
1 hour ago, matt77hias said:

I find the "Try" a bit ambiguous since the name is also associated with try/catch clauses. "GetAnim" should suffice, for the remainder (i.e. what happens if the index does not match anything?), you should read the specifications of the method, IMHO.

I disagree. try/catch has a sufficiently visually different syntax from a function invocation that I would hope nobody would ever mistake the two.

The implication of the "try" function nomenclature is that the function could fail in a recoverable way, or in a way that allows you to have different behaviour when it fails. Some would argue that exceptions are preferable here; not all of us use exceptions or even think that exceptions in C++ are a good idea. The explicit naming is useful when we want to obviously distinguish between functions that can fail and functions that cannot - eg. in the case where, for performance reasons, you want to access an animation by index without bounds checking. That difference is an important aspect of the functions' semantics.

// returns the animation with the given index, or nullptr if the index is invalid
const Anim* TryAndGetAnim(const int index) const;

// returns the animation with the given index - asserts if the index is invalid,
// this function should never be called when we don't already know that the index is valid
const Anim& GetAnim(const int index) const;

// returns the index associated with the name, or -1 if it fails to find it
int FindAnimIndex(const char* name) const;

// use cases:
if (const Anim* anim = TryAndGetAnim(FindAnimIndex("run")))
{
  //...
}

int animIndex = FindAnimIndex("run");
if (animIndex != -1)
{
  // use the index here...
  const Anim& anim = GetAnim(animIndex);
  // ...
}

Having the different name also lets you avoid the fact that you can't overload functions by return type alone, of course.

Quote

Furthermore, it could be beneficial to have multiple ways of accessing the "Anim" with regard of performance (I am not saying they should all have the same name). Of course: make it work -> make it right (refactoring) -> make it fast (profiling/optimization); so alternative ways (if beneficial) should only be added in the end.

I don't see any functional difference between GetAnim(FindAnimIndex("run")) and GetAnim("run"). The performance difference is likely meaningless, depending on how these are used, and I'd argue that the first one is much clearer than the second one even if it's more verbose. I want to be able to tell at a glance whether a search algorithm is happening or whether a simple indexed lookup is happening. Chances are good my peer reviewers (you do have a peer review process on code checkins, right?) are going to want the same thing.

Of course, one could also invert the above paradigm and change the names a bit:

// returns the animation with the given name, or nullptr if we didn't find it
const Anim* TryAndFindAnim(const char* name) const;

// returns the animation with the given name - asserts if we didn't find it,
// this function should never be called when we don't already know that the animation exists
const Anim& FindAnim(const char* name) const;

// returns the index of the animation, or -1 if it isn't in this container or is nullptr
int ComputeAnimIndex(const Anim* anim) const;

As you suggest, you could have both. In that case having different names is an absolute must; note that in my second code example I'm using "Find" rather than "Get." This is because to me "Find" implies a search operation (probably one that's O(n)) while "Get" implies a simple O(1) lookup.  Furthermore, as you suggested, I probably wouldn't write the extra functions up front, I'd wait until a need for them was demonstrated. I'd write whichever set of functions was the most common case at the time I needed the functionality.

Ultimately, this still isn't a good use case for overloading. Overloading should be used in cases where there is no semantic difference between the functions and the only difference is the types they take, as with trig functions taking floats or doubles.

Edited by Oberon_Command

Share this post


Link to post
Share on other sites
Posted (edited)
16 hours ago, Oberon_Command said:

Ultimately, this still isn't a good use case for overloading. Overloading should be used in cases where there is no semantic difference between the functions and the only difference is the types they take, as with trig functions taking floats or doubles.

More specifically, a difference in types that must be dealt with by the function, not by the caller. If the caller can convert would-be function parameters to equivalent values of other types only the "standard" parameter type should be supported.

Mathematical functions are an appropriate example: float and double computations of the "same" function are implemented differently and have different results.

On the other hand there's no reason to add complication to provide overloads over float-precision complex numbers represented as std::complex<float> or std::tuple<float,float> or std::pair<float,float> or std::array<float,2> or two floats, as these representations can all be converted trivially and exactly to each other.

Edited by LorenzoGatti

Share this post


Link to post
Share on other sites

The whole point of frameworks is so whoever is using it has less work to do. Nobody wants to learn a million tediously different functions if they do the exact same thing but are called with different parameters. There are a million ways to do the same thing, no right answer obviously, so just use your best judgement when doing things like overloading. If two functions are named the same, and do completely different things, maybe make the name a little more descriptive. 

I think the example in the OP was only a good example of a bad use of overloading, but not a great argument for why overloading in general is a bad idea.

I don't mind the "Try" function naming, but i'd prefer it to return a boolean, and have an out parameter for the return anim, basically like how TryParse works in .net

Share this post


Link to post
Share on other sites
1 hour ago, iedoc said:

I don't mind the "Try" function naming, but i'd prefer it to return a boolean, and have an out parameter for the return anim, basically like how TryParse works in .net

We're getting a bit off topic, but the advantage of the "Try" functions returning a pointer is that you can restrict the scope of the local variable used to store the function's return value to the if block used to test for the function's success. It also allows the return value to be used ONLY for validity testing (eg. when I only need to know that something exists, not to actually use it, which is useful in some cases) without having a spurious dummy variable the way the .Net "TryParse" style does.

// repeated from my post above...
// note that we can't accidentally use anim outside this block where it might be nullptr!
if (const Anim* anim = TryAndGetAnim(FindAnimIndex("run")))
{
  //...
}

// in C++'17 with non-integer types we could use an optional type
if (const std::optional<int> animIndex = TryAndFindAnimIndex("run")) 
{
  // ...
}

// we could also do this with types that don't evaluate to booleans, without optionals
if (const int animIndex = FindAnimIndex("run"); animIndex != -1) 
{
  // ...
}

// and I guess the TryParse way, too, but without this syntax, 
// if we don't need the value, or we don't have a sensible default value, that way could result in bugs...
if (const Anim* anim = nullptr; TryAndGetAnim(FindAnimIndex("run"), &anim))
{
  // ...
}

 

Edited by Oberon_Command

Share this post


Link to post
Share on other sites

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


  • Forum Statistics

    • Total Topics
      628740
    • Total Posts
      2984472
  • Similar Content

    • By Josheir
      void update() { if (thrust) { dx += cos(angle*DEGTORAD)*.02; dy += sin(angle*DEGTORAD)*.02; } else { dx*=0.99; dy*=0.99; } int maxSpeed = 15; float speed = sqrt(dx*dx+dy*dy); if (speed>maxSpeed) { dx *= maxSpeed/speed; dy *= maxSpeed/speed; } x+=dx; y+=dy; . . . } In the above code, why is maxSpeed being divided by the speed variable.  I'm stumped.
       
      Thank you,
      Josheir
    • By Benjamin Shefte
      Hey there,  I have this old code im trying to compile using GCC and am running into a few issues..
      im trying to figure out how to convert these functions to gcc
      static __int64 MyQueryPerformanceFrequency() { static __int64 aFreq = 0; if(aFreq!=0) return aFreq; LARGE_INTEGER s1, e1, f1; __int64 s2, e2, f2; QueryPerformanceCounter(&s1); s2 = MyQueryPerformanceCounter(); Sleep(50); e2 = MyQueryPerformanceCounter(); QueryPerformanceCounter(&e1); QueryPerformanceFrequency(&f1); double aTime = (double)(e1.QuadPart - s1.QuadPart)/f1.QuadPart; f2 = (e2 - s2)/aTime; aFreq = f2; return aFreq; } void PerfTimer::GlobalStart(const char *theName) { gPerfTimerStarted = true; gPerfTotalTime = 0; gPerfTimerStartCount = 0; gPerfElapsedTime = 0; LARGE_INTEGER anInt; QueryPerformanceCounter(&anInt); gPerfResetTick = anInt.QuadPart; } /////////////////////////////////////////////////////////////////////////////// /////////////////////////////////////////////////////////////////////////////// void PerfTimer::GlobalStop(const char *theName) { LARGE_INTEGER anInt; QueryPerformanceCounter(&anInt); LARGE_INTEGER aFreq; QueryPerformanceFrequency(&aFreq); gPerfElapsedTime = (double)(anInt.QuadPart - gPerfResetTick)/aFreq.QuadPart*1000.0; gPerfTimerStarted = false; }  
      I also tried converting this function (original function is the first function below and my converted for gcc function is under that) is this correct?:
      #if defined(WIN32) static __int64 MyQueryPerformanceCounter() { // LARGE_INTEGER anInt; // QueryPerformanceCounter(&anInt); // return anInt.QuadPart; #if defined(WIN32) unsigned long x,y; _asm { rdtsc mov x, eax mov y, edx } __int64 result = y; result<<=32; result|=x; return result; } #else static __int64 MyQueryPerformanceCounter() { struct timeval t1, t2; double elapsedTime; // start timer gettimeofday(&t1, NULL); Sleep(50); // stop timer gettimeofday(&t2, NULL); // compute and print the elapsed time in millisec elapsedTime = (t2.tv_sec - t1.tv_sec) * 1000.0; // sec to ms elapsedTime += (t2.tv_usec - t1.tv_usec) / 1000.0; // us to ms return elapsedTime; } #endif Any help would be appreciated, Thank you!
    • By mister345
      Hi, I'm building a game engine using DirectX11 in c++.
      I need a basic physics engine to handle collisions and motion, and no time to write my own.
      What is the easiest solution for this? Bullet and PhysX both seem too complicated and would still require writing my own wrapper classes, it seems. 
      I found this thing called PAL - physics abstraction layer that can support bullet, physx, etc, but it's so old and no info on how to download or install it.
      The simpler the better. Please let me know, thanks!
    • By lawnjelly
      It comes that time again when I try and get my PC build working on Android via Android Studio. All was going swimmingly, it ran in the emulator fine, but on my first actual test device (Google Nexus 7 2012 tablet (32 bit ARM Cortex-A9, ARM v7A architecture)) I was getting a 'SIGBUS illegal alignment' crash.
      My little research has indicated that while x86 is fine with loading 16 / 32 / 64 bit values from any byte address in memory, the earlier ARM chips may need data to be aligned to the data size. This isn't a massive problem, and I see the reason for it (probably faster, like SIMD aligned loads, and simpler for the CPU). I probably have quite a few of these, particular in my own byte packed file formats. I can adjust the exporter / formats so that they are using the required alignment.
      Just to confirm, if anyone knows this, is it all 16 / 32 / 64 bit accesses that need to be data size aligned on early android devices? Or e.g. just 64 bit size access? 
      And is there any easy way to get the compiler to spit out some kind of useful information as to the alignment of each member of a struct / class, so I can quickly pin down the culprits?
      The ARM docs (http://infocenter.arm.com/help/index.jsp?topic=/com.arm.doc.faqs/ka15414.html) suggest another alternative is using a __packed qualifier. Anyone used this, is this practical?
    • By Josheir
      In the following code:

       
      Point p = a[1]; center of rotation for (int i = 0; I<4; i++) { int x = a[i].x - p.x; int y = a[i].y - p.y; a[i].x = y + p.x; a[i].y = - x + p.y; }  
      I am understanding that a 90 degree shift results in a change like:   
      xNew = -y
      yNew = x
       
      Could someone please explain how the two additions and subtractions of the p.x and p.y works?
       
      Thank you,
      Josheir
  • Popular Now