• TheCPUWizard (unregistered)

    Not conclusively: The new instance takes "this" as a parameter, and it is possible that is code not shown there is "ownership and lifetime management".

    IF this is the case then deleting the object within this method may result in double deletion.

    Be careful about assumptions.

  • (nodebb)

    I've never seen so much unreachable code in one place in my life.

  • TopTension (unregistered)

    The DeleteIfNotNull is also useless. The specification for delete (since C++98) states, that deleting a nullpointer is allowed and does nothing.

  • (nodebb)

    For reference, the magic number is RPC_E_FAULT, meaning "RPC could not call the server or could not return the results of calling the server."

    See https://learn.microsoft.com/en-us/openspecs/windows_protocols/ms-erref/705fb797-2175-4a90-b5a3-3918024b10b8

    if FAILED is the result

    Observation: the if() doesn't mean that. It means "if the HRESULT returned from RetrieveDetailsUsingOutlook97() was one of the two gibivalues that indicate a failure". It's still WTF to call the function again with identical parameters, mind you, but the editing of the submission is also somewhat WTF.

    For reference, an HRESULT is a 32-bit unsigned integer type returned mostly from COM API calls and from calls to COM objects. It is defined so that if the high bit is 0, the return value indicates success, and if the bit is 1, the return value indicates failure. There aren't many named success values, and S_OK is probably the most commonly-used one (value 0. But there are others, and the one that is most guilty of WTF-ness is S_FALSE, whose value is 1!

    But in the 2020s, if someone is still using HRESULTs (or, worse, COM objects), that person is TRTRTRWTF.

  • Lothar (unregistered)

    Maybe the FAILED macro returns true if there was no failure

  • (nodebb)

    More importantly, "p_cio_Local" is never used in any way, so why was it created?

  • Sauron (unregistered)

    An enforceable code review policy first requires that people care.

    Many devs just don't bother reviewing other devs' code.

    Among those who do review, sometimes they only have a very superficial look, and when you give them a commit of 20000 lines with deep changes to the code and to the database, they add couple of useless comments "You put 2 spaces between that bracket and that curly brace", rather than looking for the really important stuffs like whether there's proper access control, whether the code properly handles errors, whether the database is properly structured, whether the logic flows well, etc.

    So, having a good code review is damn valuable, but also rare.

    Also, management is super happy to mention the code reviews as part of the quality of the process when they're audited, but in the actual day-to-day work, they want to rush the shipping of the releases by every possible mean, and as a result the review isn't done as properly as it should.

  • Alexander J. Vincent (github)

    ... and this is why we have smart pointers (with destructors to clean up memory), to save us from dumb programmers.

  • Duke of New York (unregistered) in reply to Alexander J. Vincent

    C++'s essential defect is that it makes doing the right thing with memory easy, but it does not make doing the wrong thing hard.

  • TF (unregistered)

    The first function might not be so bad if there is some implied ownership or other specialized memory management involved in passing this to the constructor of the new object. But the fact that your first instinct is to suspect a typical memory leak mistake rather than understanding that specialized ownership is a problem.

  • Argle (unregistered) in reply to Alexander J. Vincent

    Decades of programming have taught me that there is nothing that will save us from dumb programmers. As the saying goes, make something idiot proof and nature makes a better idiot. This applies to programming.

  • (author) in reply to TF

    But there isn't any sort of implied ownership- we new an object, and let the pointer go out of scope before deleteing it. Could they have overridden new in some fashion? Maybe changing their whole program to use Arena Allocation or something? Sure, it's possible. But did they? Almost certainly not.

  • (nodebb) in reply to TF

    But the fact that your first instinct is to suspect a typical memory leak mistake rather than understanding that specialized ownership is a problem.

    Excessive use of specialised ownership is, indeed, a problem, because it isn't visible at the point of use.

    In fact, I'm tempted to suspect two things:

    • The most probable explanation if it doesn't leak is that the constructor passes the new object itself to the "parent" object, which stuffs a pointer to the new object in a container full of smart pointers.
    • The name if the two variables speaks against that, and in any event there should be an "unreachable code" warning generated by the delete, so either they have deactivated -Werror or equivalent, or, more likely, they have deactivated this and probably many other warnings.
  • JJ (unregistered)

    So we're writing COM code, which is why you're seeing the double pointer. This is not a WTF. The logic is :-)

    The SUCCEEDED(hr) and FAILED(hr) macros are the proper way to handle success and failure in COM. Every COM error is negative and every success positive, so these are quite small macros :-)

    The magic number is RPC_E_FAULT. With the right include, the symbol exists. This is an RPC lib error ("could not call server or return results from call")..

  • Gavin (unregistered) in reply to Remy Porter

    Remy is right, that function is exactly as it appears in the code base. There is no implied ownership anywhere. WYSIWYG

  • Duke of New York (unregistered)

    As Guy Steele's lambda papers explain, contorting your code to have "one point of return" is futile, because the only (normal) return point is whatever the caller pushed on the stack.

  • Klimax (unregistered)

    "Then we try and do an operations, and if FAILED is the result, we… return the result. We don't return the result if it succeeded. " Sorry, Remy but here you are wrong. Code is returning HRESULT that contains error code. There is going to be return (it is after all mandatory), so that return will, well, return result code in hr.

    Basic WinAPI/COM coding.

  • Klimax (unregistered) in reply to Steve_The_Cynic

    Beside personal hatred, why would you say it about person writing COM code or dealt with HRESULT. WinAPI is still very alive and not only there are plenty of COM objects in Windows, but there are newer versions of it too. (Like WinRT)

  • (nodebb)

    The mix of coding styles is another code smell. And who give variables names like p_cio_Folder, p_mrl_To etc. ? It's neither camelCase, nor PascalCase, nor snake_case.

  • mihi (unregistered)

    Ignoring HRESULTs that are not FAILED is not necessarily a code smell. Many COM functions can only return either S_OK or an error code. So if it is not FAILED, it was S_OK and you can happily continue torturing MS Outlook via a COM interface until you get the next error - or if you got none, return S_OK.

  • ismo (unregistered)

    While the DeleteIfNotNULL( p_cio_Local ); is never called it is itself also code smell. Deleting null pointer is always valid, well defined in standard. The code should use std::unique_ptr or something similar. On never use new at all as it is stupid for local variables which have well defined lifetime ( if the object is very big then mayby heap variable is ok).

  • Morten Jørgensen (unregistered) 
    if ( uiMessageType == MT_SENT_SMS )
{
	if ( uiMessageType != MT_SENT_SMS ) POLL_TERMINATE_EVENT_SOK();
	hr = AddAllRecipients( pal, iNumEntries, p_mrl_To, p_mrl_Cc );
	if ( FAILED( hr ) ) return hr;
}
else
{
	if ( uiMessageType != MT_SENT_SMS ) POLL_TERMINATE_EVENT_SOK();
}

    We do not know the type of uiMessageType and as such we do not know that == and != are the opposite. The logic implemented may make sense. I doubt it, but it just may make sense.

    Even if == and != are each others logic opposites, we dont know if == changes state of uiMessageType so the following != may be needed.

  • (nodebb)

    Nobody seems to have noticed that the else branch in the second example also retests uiMessageType

    if ( uiMessageType == MT_SENT_SMS )
{
	// snip
}
else //  uiMessageType != MT_SENT_SMS
{
	if ( uiMessageType != MT_SENT_SMS ) POLL_TERMINATE_EVENT_SOK();
}

Leave a comment on “Many Unhappy Returns”

Log In or post as a guest

Replying to comment #:

Log Out

« Return to Article