Re: [boost] [Boost.Breakable] Any interest in a Boost Breakable library?

Subject: Re: [boost] [Boost.Breakable] Any interest in a Boost Breakable library?
From: Stewart, Robert (Robert.Stewart_at_[hidden])
Date: 2009-09-09 12:17:55

Ilya Bobir wrote:
> Stewart, Robert wrote:
> > Ilya Bobir wrote:
> >> Here is a real life sample I was able to find using Google
> >> Code search:
> >> http://tinyurl.com/mlc6zo
> >> We are trying to load a cursor.  There are 3 different
> >> points were it
> >> can fail.  And we want to return a default cursor in case of
> >> any failure.
> >
> > That's a horrible example.  The loop is used to jump to
> > cleanup code that should be handled using RAII (a sentry
> > would work nicely).
>
> While I do think that RAII is a very good approach it just
> does not do
> well in case you have to deal with a C style API.  Windows
> API is mostly
> C style.  As well as the old DirectX API.

I do whatever I can to avoid dealing with C APIs.

> If all APIs were C++ and were codded to support RAII and exceptions
> programming in C++ would be a lot easier but it is not the
> case.  Some
> of C++ programmers still have to deal with C APIs and wrapping
> everything to support RAII is not always the most cost
> effective approach.

I much prefer RAII even when not dealing with exceptions.  It makes the code clearer.

> >> Another context were this kind of logic may appear are
> >> parsers.  Here is
> >> another real life sample: http://tinyurl.com/ljq5n9
> >> I do not think that "cutting out" the "do { ... } while
> >> (false);" part
> >> into a separate function would add readability.
> >
> > It would work nicely to create a separate function.  The
> subfunction can return early with a flag that indicates
> failure and the main function can assign to status and return
> 0.0.  Which is clearer would depend upon the reader, I suppose.
>
> This way you may end up with a lot of functions that are named very
> similarly and call each other doing only very small pieces of work.
> While generally it seems to be a good thing when your functions are
> short when they became too short, because you are blindly adhering to
> some coding practice, the code looses readability.

That can happen, but its rare.

> I have seen some prominent examples in the Windows source of
> a code that
> is hard to understand because a simple thing is performed by
> a dozen of
> functions that call each other.  It is hard to understand what a
> separate function does as the code is very tightly coupled
> but because
> of some coding convention it is split into different
> functions.  I can
> not give you a direct link as the Windows source does not seems to be
> available online, but here is a stack trace of a Windows Shell call:
>
> shell32.dll!AicpMsgWaitForCompletion()  + 0x36
> shell32.dll!AicpAsyncFinishCall()  + 0x2c
> shell32.dll!AicLaunchAdminProcess()  + 0x2ee
> shell32.dll!_SHCreateProcess()  + 0x59d0
> shell32.dll!CExecuteApplication::_CreateProcess()  + 0xac
> shell32.dll!CExecuteApplication::_TryCreateProcess()  + 0x2e
> shell32.dll!CExecuteApplication::_DoApplication()  + 0x3c
> shell32.dll!CExecuteApplication::Execute()  + 0x33
> shell32.dll!CExecuteAssociation::_DoCommand()  + 0x5b
> shell32.dll!CExecuteAssociation::_TryApplication()  + 0x32
> shell32.dll!CExecuteAssociation::Execute()  + 0x30
> shell32.dll!CShellExecute::_ExecuteAssoc()  + 0x82
> shell32.dll!CShellExecute::_DoExecute()  + 0x4c
> shell32.dll!CShellExecute::s_ExecuteThreadProc()  + 0x25
> shlwapi.dll!WrapperThreadProc()  + 0x98
> kernel32.dll!@BaseThreadInitThunk_at_12()  + 0x12
> ntdll.dll!__RtlUserThreadStart_at_8()  + 0x27
>
> The call is supposed to start a process.  As you can see method names
> are very similar and, I believe, unless you are familiar with
> the code
> it is not obvious what each function does and unless you have a stack
> trace it is not even obvious the order in which the functions
> are called.

Without details on how complex those functions are, neither of us can comment on this example.  If the functions in that stack trace are non-trivial, if they are reused, etc., the factorization was a good idea.  If they are tiny functions that take lots of arguments, then the factorization was a bad idea.

> > The point is that one *can* always rewrite the code a
> > different way.  Perhaps the "breakable" approach makes
> > certain code clearer, but it seems to apply to code that is
> > already not exception safe and very C-like.
>
> Totally agreed.  Note that not all the code out there is a proper C++
> style and there are billions lines of code already written.

When maintaining code, one often has the choice of inserting something like the "breakable" approach, refactoring, adding RAII, etc.

> >> Cretan languages support this "breakable" construct natively.
> >> OvermindDL1 wrote that D have something similar and it was designed
> >> quite recently.  I can add that Perl have direct support for
> >> this kind
> >> of control flow.  In Perl it looks like this:
> >
> > That other languages do something doesn't mean it is a good
> > idea, nor does its being in Perl mean it shouldn't be in C++. ;)
>
> The point it that the breakable is a control flow construct already
> "accepted" by some languages.  And as such it can be treated as a
> control flow pattern.  Not as common as "for" but still a
> control flow
> pattern, not a "bad coding style".

"That other languages do something doesn't mean it is a good idea."  Just because it is codified in other languages as a control flow pattern doesn't mean it isn't bad coding style.

We seem to be agreeing violently that the "Breakable Idiom" may make some code clearer but that there are many alternatives that bring other benefits, so I'll not say more.  The question is whether Boost will accept it.  The only way to really know the answer to that question is to have it reviewed.

_____
Rob Stewart                           robert.stewart_at_[hidden]
Software Engineer, Core Software      using std::disclaimer;
Susquehanna International Group, LLP  http://www.sig.com

IMPORTANT: The information contained in this email and/or its attachments is confidential. If you are not the intended recipient, please notify the sender immediately by reply and immediately delete this message and all its attachments. Any review, use, reproduction, disclosure or dissemination of this message or any attachment by an unintended recipient is strictly prohibited. Neither this message nor any attachment is intended as or should be construed as an offer, solicitation or recommendation to buy or sell any security or other financial instrument. Neither the sender, his or her employer nor any of their respective affiliates makes any warranties as to the completeness or accuracy of any of the information contained herein or that this message or any of its attachments is free of viruses.