Returning a nullptr refence, how bad is my teammate?

0.

Your compiler should be warning you that you're returning a reference to a temporary. If it isn't, get a better one, and make warnings errors.

I am very interested what is your opinion on returning a reference that is nullptr.

That's not what that code is doing, per se; that code is creating a std::string temporary out of the null pointer (via the constructor that takes a const char *) and returning that.

how bad is my teammate?

I wouldn't be too quick to judge, it would be easy for this code to come into being via a refactor that changed to std::string from using C-style const char* strings, or any other number of incremental changes. The real problem is that you're not compiling with the tools to catch these issues (or you are, and you're letting people check in with compile errors or without compiling). Either way the larger failure is on the process, and calling him a bad programmer isn't (necessarily) fair.

There are other flaws with the code as well (you search the map twice, once in the find call, and then redundantly again in the bottom-most return call where you can instead make use of the iterator).

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).

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

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

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

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.

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...

Your compiler should be warning you that you're returning a reference to a temporary. If it isn't, get a better one, and make warnings errors.

We've probably been over this before, but why isn't this covered by the most important const thingymajig?

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.)

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.

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)

We've probably been over this before, but why isn't this covered by the most important const thingymajig?

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.

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
}

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;
}