Sign in to follow this  
BaneTrapper

Returning a nullptr refence, how bad is my teammate?

Recommended Posts

BaneTrapper    1531

Does anyone find this code being very bad?

Can you rate this from 0 to 10. I am very interested what is your opinion on returning a reference that is nullptr.

const std::string &SoundHandler::getPlayingFromPlaylist(const std::string& playlistName)
{
    //Check to see if the playlist exists
    auto iter = playlists.find(playlistName);
    if(iter == playlists.end())
        return nullptr;

    return playlists[playlistName].currentlyPlaying;
}

Share this post


Link to post
Share on other sites
rip-off    10979
Ordinary, such code would not compile, but in this case I believe this will attempt to construct a temporary std::string (as if from a null character pointer) which most implementations will assert/guard against, but either way this an express trip through undefined behaviour land, likely destination Teh Crash.

Nice of them to test the function thoroughly though, in addition to compiling on an insufficient warning level (and without warnings as errors).

Share this post


Link to post
Share on other sites
BaneTrapper    1531

I never meant it as an insult, i just couldn't force him to change it happy.png . I expect references to be valid, if he cannot ensure it, return a pointer.

After a laugh he did a quick fix cool.png , thx on the opinions.

EDIT:: The compiler gave a warning, he though its okay.

Edited by BaneTrapper

Share this post


Link to post
Share on other sites

In situations where it can optionally return nothing, I'd prefer something list std::optional / boost::optional.

 

What I actually do in these situations, is return a const reference to a static empty version and log a warning/error. Not ideal, but better than a lying reference. However, I've been deciding more and more that asserts are my friend.

Share this post


Link to post
Share on other sites
Hodgman    51341

that code is creating a std::string temporary out of the null pointer (via the constructor that takes a const char *) and returning that.

I thought it would be something like that... Except isn't nullptr supposed to solve this problem (compared to NULL/0) where null pointers weren't typesafe and thus caused people to unknowingly call the wrong function?

(edit) brain fart - nullptr prevents it being passed as an int, but has to be able to implicitly convert to any/all pointer types...

Share this post


Link to post
Share on other sites

Returning nullptr there will invoke undefined behavior as nullptr does not point at an array of at least traits::length(s)+1 elements of CharT, which is a requirement of this overload.

 

There was probably good intent behind this (so I'm still giving 2 points rather than 0), assuming that std::string would make something meaningful (empty string, or "the null string") out of it, but it's really just undefined behavior.

 

(The reference to temporary thing isn't really much of an issue since it's const, so its lifetime is extended to the lifetime of the reference.)

Edited by samoth

Share this post


Link to post
Share on other sites
l0calh05t    1797

EDIT:: The compiler gave a warning, he though its okay.

 

That is why I love -werror or /WX. Reduces the chances of idiot programmers to ignore the warnings.

Edited by l0calh05t

Share this post


Link to post
Share on other sites
ferrous    6140

 


EDIT:: The compiler gave a warning, he though its okay.

 

That is why I love -werror or /WX. Reduces the chances of idiot programmers to ignore the warnings.

 

 

It's really easy to ignore warnings, or just miss them if you don't seem them.  Having warnings as errors can seem like a pain, especially if turned on more than halfway through a project, but it's totally worth doing.  (And it's best done from the beginning)

Share this post


Link to post
Share on other sites
jpetrie    13161
We've probably been over this before, but why isn't this covered by the most important const thingymajig?

 

 
(The reference to temporary thing isn't really much of an issue since it's const, so its lifetime is extended to the lifetime of the reference.)

 

 
Only local const references extend the lifetime of a temporary. In Sutter's first example there, f() is returning a string by value; a copy of the string constructed from "ABC" is on the stack and the reference "s" extends the lifetime of that copy to the scope of s itself. That doesn't apply in the above situation (which is why the compiler warns about it).
 
The standard, in 12.2/5 (class.temporary) says that "the lifetime of a temporary bound to the returned value in a function return statement (6.6.3) is not
extended; the temporary is destroyed at the end of the full-expression in the return statement."

Share this post


Link to post
Share on other sites
Washu    7829
Additionally, if you're using a remotely modern compiler (i.e. 2012 and beyond), there's no reason to return std::string as anything but a value as with move constructors there is no copy. Edited by Washu

Share this post


Link to post
Share on other sites
Juliean    7077

Additionally, if you're using a remotely modern compiler (i.e. 2012 and beyond), there's no reason to return std::string as anything but a value as with move constructors there is no copy.

 

Move constructors only work on temporaries, so if you return a string that is already stored somewhere else (like in the OP code), then returning by value would still invoke a copy.

std::wstring function(const std::wstring& input)
{
	return input + L".prefix"; // move constructor invoked
}

std::wstring function(const std::wstring& key)
{
	static std::map<std::wstring, std::wstring> map;
	
	return map[key]; // makes a copy of the value stored in the map, so returning by reference is still faster
}
Edited by Juliean

Share this post


Link to post
Share on other sites
l0calh05t    1797

Yes, in this case it would cause a copy. As servant of the lord suggested, a reference to a static or global object could be returned (and as the reference is const, the usual issues with static or global variables don't apply.

 

But the code isn't written in an efficient manner anyways, as first find is called to check if the object exists... and then the [] operator does an internal find again. If you are already have an iterator use it!

static const std::string empty;
const std::string& SoundHandler::getPlayingFromPlaylist(const std::string& playlistName)
{
    //Check to see if the playlist exists
    auto iter = playlists.find(playlistName);
    if(iter == playlists.end())
        return empty;

    return iter->currentlyPlaying;
}
Edited by l0calh05t

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

Sign in to follow this