Search code examples
c++c++11static-analysisc++98prefast

Does this pattern work in C++?


I'm running PREfast static code analysis on my our projects, and it is giving me C6001 'using uninitialized memory' errors for this pattern:

// AutoSelectGDIObject is a class
if (AutoSelectGDIObject & select_image = AutoSelectGDIObject(hDCImgSource, hBmp))
{
   // use hDCImgSource knowing that hBmp is selected into it
}
// now we are guaranteed that hDCImgSource has had hBmp removed and its previous bmp re-selected into itself

The trick I'm trying to exploit is to allow the scoping of select_image to just the if-expression (i.e. if (condition) { expression-block = lifetime of condition variable }.

VS has happily compiled (and presumably run this) for quite some time. I haven't single stepped code like this in a long time, but as far as I can tell, it only enters the if block if select_image's operator bool() returns true, and it destroys the instance of select_image upon exiting that if block.

Is PREfast wrong? Or is there something subtle here that makes my above code and assumptions incorrect?


Solution

  • Is PREfast wrong? Or is there something subtle here that makes my above code and assumptions incorrect?

    That code is invalid C++, but VC compiles it because it allows, as a documented extension, binding lvalue references to non const-qualified types to temporaries.

    This is a silly extension, in my opinion. According to the C++ Standard, temporaries can only be bound lvalue references to const or to rvalue references (and in this case, their lifetime is extended beyond the end of the full-expression that creates them).

    Therefore, your if statement should be:

    if (AutoSelectGDIObject const& select_image = 
    //                      ^^^^^
                                   AutoSelectGDIObject(hDCImgSource, hBmp))
    

    See, for instance, this live example.

    Notice, however, that you should not need to use references at all here. In other words, the following if statement is also valid, and expresses your intention better than creating a temporary bound to an lvalue reference to const:

    if (AutoSelectGDIObject select_image = AutoSelectGDIObject(hDCImgSource, hBmp))
    

    See, for instance, this live example. Besides, the above would also allow you to modify select_image, while the reference to const will not.