The following code demonstrate the similar problem I found during a
code review:

class Foo
{
public: bool bar();
private: // The contents is irrelevant to the question
};

int main(int argc, char **argv)
{
    Foo *pFoo =new Foo;
    if (!pFoo->bar())
    {
        delete pFoo;
        // Log the failure
        return 1;
    }
    delete pFoo;
    return 0;
}

I would suggest the Foo object should be allocated from stack to
achieve exception safety. Like this ...

int main(int argc, char **argv)
{
    Foo foo;
    if (!foo.bar())
    {
        // Log the failure
        return 1;
    }
    return 0;
}

But my colleague claims the code is copied and pasted from some other
module already working on production. We shouldn't change the pattern.
Or should we ? Could the suggested refactoring possibly break
anything ?


--
      [ See http://www.gotw.ca/resources/clcm.htm for info about ]
      [ comp.lang.c++.moderated.    First time posters: Do this! ]

* Buck Yeh:
> The following code demonstrate the similar problem I found during a
> code review:
> 
> class Foo
> {
> public: bool bar();
> private: // The contents is irrelevant to the question
> };
> 
> int main(int argc, char **argv)
> {
>     Foo *pFoo =new Foo;
>     if (!pFoo->bar())
>     {
>         delete pFoo;
>         // Log the failure
>         return 1;
>     }
>     delete pFoo;
>     return 0;
> }

This is an anti-pattern, two-phase initialization.  The code says the 
object is unusable unless bar().  In the context of well-designed other 
code that handles exceptions, that check should be in the class' 
constructor, which should throw if not bar().


> I would suggest the Foo object should be allocated from stack to
> achieve exception safety. Like this ...
> 
> int main(int argc, char **argv)
> {
>     Foo foo;
>     if (!foo.bar())
>     {
>         // Log the failure
>         return 1;
>     }
>     return 0;
> }
> 
> But my colleague claims the code is copied and pasted from some other
> module already working on production. We shouldn't change the pattern.
> Or should we ? Could the suggested refactoring possibly break
> anything ?

Yes.  Assuming the code isn't really in 'main', but in a function 'bar', 
and that the Foo object is used in some way before it's deleted: 
consider where some of the code called before delete stores a pointer to 
the object in some global data structure and throws an exception that 
effectively (but probably unwittingly) avoids the delete in 'bar'.  The 
original code would then work, the changed code would not.

Unless you're prepared to check whether that situation occurs and fix 
that in turn, don't fix.


-- 
A: Because it messes up the order in which people normally read text.
Q: Why is it such a bad thing?
A: Top-posting.
Q: What is the most annoying thing on usenet and in e-mail?

      [ See http://www.gotw.ca/resources/clcm.htm for info about ]
      [ comp.lang.c++.moderated.    First time posters: Do this! ]

In article
<b10db5ad-4d62-4f74-ba47-3ae88cf7a434@b40g2000prf.googlegroups.com>,
Buck Yeh <buck.yeh@gmail.com> wrote:

> The following code demonstrate the similar problem I found during a
> code review:
> 
> class Foo
> {
> public: bool bar();
> private: // The contents is irrelevant to the question
> };
> 
> int main(int argc, char **argv)
> {
>     Foo *pFoo =new Foo;
>     if (!pFoo->bar())
>     {
>         delete pFoo;
>         // Log the failure
>         return 1;
>     }
>     delete pFoo;
>     return 0;
> }
> 
> I would suggest the Foo object should be allocated from stack to
> achieve exception safety. Like this ...
> 
> int main(int argc, char **argv)
> {
>     Foo foo;
>     if (!foo.bar())
>     {
>         // Log the failure
>         return 1;
>     }
>     return 0;
> }
> 
> But my colleague claims the code is copied and pasted from some other
> module already working on production. We shouldn't change the pattern.
> Or should we ? Could the suggested refactoring possibly break
> anything ?
> 
    Depends on the pasted code.

    case 1) code does not delete anything then
       then having a Foo on the stack and passing a pointer to
       the pasted code should be fine.
    case 2) code takes ownership of pointer and deletes it.
          then an auto_ptr in main with a release before the pasted
          code should work,
    case 3) pasted code deletes sometimes and indicates it to
       the calling function then you  release the auto_ptr and reset
it if the code indicates it was not deleted.
     case 4)  pasted code deletes blindly [does not indicate to calling
    function it deleted the ptr] there is no solution other than care in
the original code.

     Does this make sense??

-- 
      [ See http://www.gotw.ca/resources/clcm.htm for info about ]
      [ comp.lang.c++.moderated.    First time posters: Do this! ]