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

  • (nodebb) in reply to Steve_The_Cynic

    Oh good, I was wondering if I'm the only one how wouldn't use HRESULT in an C++ app beyond some COM+ compatibility wrapper.

  • 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)
    Comment held for moderation.
  • Klimax (unregistered)
    Comment held for moderation.
  • Klimax (unregistered) in reply to Steve_The_Cynic
    Comment held for moderation.
  • (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)
    Comment held for moderation.
  • Morten Jørgensen (unregistered)
    Comment held for moderation.
  • mataj (unregistered)
    Comment held for moderation.
  • (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();
}
  • Ternary King (unregistered)
    Comment held for moderation.
  • Ryan (unregistered)
    Comment held for moderation.

Leave a comment on “Many Unhappy Returns”

Log In or post as a guest

Replying to comment #:

Log Out

« Return to Article