Issue about lifetime of temporary in src/XMLAttributes.cpp

Issue #1086 resolved
Christopher Beck
created an issue

When compiling CEGUI as C++11 using clang 3.5, I get a warning about returning a reference to a local temporary object:

https://bitbucket.org/cegui/cegui/src/cee55f73f6f94eb66dc21f6bcbc3cc45108a5a35/cegui/src/XMLAttributes.cpp?at=default#cl-107

I was a bit surprised so I made this stack overflow post: http://stackoverflow.com/questions/30518893/safe-to-return-const-to-object-passed-by-const

I thought that the code there was reasonably safe and the warning was safe to ignore, but the stack overflow folks seem to say that it's not a good way to do things.

The usage they are pointing out as problematic, where "def" is a constant string passed as an argument, seems to be how it's commonly used in cegui code.

I guess what I would suggest is

1) Change the function to return a String rather than const String &. I don't think this will make a performance difference, esp. in C++11 with move constructors.

2) Disable the warning, although it's usually a pretty good warning to have. You could put a #pragma gcc diagnostic ignored "-Wno-return-stack-address" in this file I guess. Most people disable warnings in libraries though so maybe there's no point to this.

It depends how worried you are that someone on your team will bind the results of this function to a local const String & some day I guess.

Comments (7)

  1. Lukas Meindl

    About 2) - Disabling a warning should only be done if really necessary. I personally don't disable warnings for libraries, a library should not throw warnings and catch all false flag warnings with pragmas internally. A fix would be nicer

    About 1) Sure, this is something we wanna do anyways. However, we support C++11 only in default branch. Not in v0-8. That said, changing the return type would break ABIwhich means the other fix can't go into v0-8 even if it was C++. So this is not a solution we can apply to v0-8. Maybe there is a workaround for v0-8 and the fix can be applied to default branch thereafter.

    Nice stackoverflow question, I upvoted it with my few hard-earned points.

    I am quite confused about the comments to the accepted answer. I do not know how to go on about solving this without breaking the ABI. Also it is really hard for me to see the "dangling reference" that is created here.

  2. Christopher Beck reporter

    I see, so what is the branch system in CEGUI? I guess default is the thing that is usually called "master" in git? And v0-8 is the most recent 0.8.x version release?

    If I understand correctly you probably can't "fix it" (make the code sample in MattMcNabb's answer work) without breaking ABI.

    My understanding is like this:

    Usually in C++ you do things like:

    std::string str = "foo";

    for a local variable -- if you do

    std::string & str = "foo";

    then if you try to use str, you segfault, because it's a dangling reference. The "foo" temporary gets destroyed at the end of the "full expression" that contains it, and str is left pointing to nothing.

    In C++ as a bizarre special extension, if the reference is const then the above works:

    const std::string & str = "foo";

    is okay and it is legal to refer to str later -- because making a const & extends the lifetime of the temporary. (http://herbsutter.com/2008/01/01/gotw-88-a-candidate-for-the-most-important-const/)

    However, what the stack overflow post is about is whether this works --through a function--. If I do

    const std::string & str = getXMLAttribute("Image", "foo.png");

    and then I try to use str, it turns out that is a segfault I guess -- even though C++ normally extends the lifetime of a temporary object, in this case it won't because there are really multiple temporaries being bound to eachother inside the function.

    So I think the correct thing is either

    1.) "Only return a full std::string in cases like this and rely on copy ellision by the compiler not to make useless copies" (and break ABI), or

    2.) "Return a reference counted pointer" (and break ABI) or

    3.) "Well I just assume that all my colleagues are good at C++ and they know that if they recieve a const std::string & from a function they had better store it in a std::string, and if they do otherwise and get segfaults well they'll learn their lesson." (but then you still have the compiler warning)

  3. Lukas Meindl

    The warning could still be bogus. I replied to the stackoverflow question in the comments. What they want is a minimal working-sample to reproduce the issue.

    I guess we should provide one and see what they say? I don't have your compiler and overall setup and you said this only appears in clang. Would you be so kind to produce a MCVE for them? That would be lovely.

    PS: The branches are described in the readme (see the https://bitbucket.org/cegui/cegui page)

  4. Lukas Meindl

    I looked at it again and this is terrible. And now I also understand the issue better. The reason why this is extra bad is that the declaration states this:

    const String& getValueAsString(const String& attrName, const String& def = "") const;
    

    So we can't even rely on the user as the default argument here may cause issues if returned. Pretty awful. This should nevertheless not be a problem in people's applications.

    To solve this I will make this a pass by value instead of reference in default branch. Still not sure what to do in v0-8, I guess we will have to leave it like it is so as to not break compatibility for people who all seem not have issues with this call anyways.

  5. Log in to comment