W_ERROR_HAVE_NO_MEMORY and friends

[simo]

On Wed, 2012-05-30 at 21:25 +1000, Andrew Bartlett wrote: 
> On Wed, 2012-05-30 at 12:35 +0200, Kai Blin wrote:
> > On 2012-05-30 07:59, Volker Lendecke wrote:
> > 
> > >>        via  6a1ad76 s4-dns: Use W_ERROR_HAVE_NO_MEMORY in create_response_rr
> > > 
> > > Where is the rule laid down that we must use those macros?
> > 
> > I'm not aware of any rule, but the rest of the DNS server code is using
> > that macro, so I thought consistency was a good thing.
> > 
> > > I find them completely obnoxious to use. Doing a return or
> > > goto from something that looks like a function call is just
> > > wrong to me. To save precious screen space, I would rather
> > > go and introduce a special rule to say
> > > 
> > > if (ptr == NULL) { return WERR_NOMEM };
> > 
> > This is not about screen space for me, it's about having to write less
> > boilerplate code. Code completion works better on the macro.
> 
> I've written code that uses it, and code that does not.  One of the
> advantages is that it promotes efficient and correct error handling
> (that is, checking every error, perhaps freeing a temp mem context). 
> 
> I tend to find that code that uses this has correct error handling,
> while code that doesn't (and so has the manually expanded version) tends
> to have poorer handling, often missing details. 

I found that this is true only after the first pass, in time when
changes pile up this kind of code tend to introduce horrible leeks when
not outright flow errors.

But the worst thing is that it kills readability.
goto and return keywords are normally highlighted in text editors that
can do syntax highlighting, making it easy to spot them. Not so with
macros.

> > > in one line. I know we have tons of uses of those, but I
> > > would like to start a discussion about banning them.
> > 
> > If you want to ban them, go ahead. I don't feel strongly enough about
> > this either way. As I said, my change was mostly to keep the
> > allocation-error checks consistent, and it was easier to change this way
> > around.
> 
> As the guideline eludes to, we do use this a lot in the torture code -
> torture_assert() actually does a return, and this is the fundamental
> basis of modern torture code.  (You can write torture_fail(), but this
> is the current best practice). 

Torture/Test code can be exempted, as it is a special purpose piece of
code, and we do not care as much about leaks and other resource
mishandling that is important long running process errors there.

> When I first saw the NT_STATUS_HAVE_NO_MEMORY macro, I didn't like it.
> I certainly had the same concerns as Volker.  It does make the code more
> compact, and tight code can be easier to grasp, but it's not a universal
> rule. 
> 
> That said, I prefer it over the goto failed style. 
> 
> The one thing I would say is that our coding guidelines should reflect
> the bulk of the recent, modern code we write and work with.  That is, I
> honestly don't read our style guide often, but I'm very much influenced
> by the style of the code I work on.  (Hence picking up
> talloc_stackframe() in source3 code, HAVE_NO_MEMORY in code metze has
> been involved with etc). 

This is bad, in fact reading samba code these days is quite painful. Not
as bad as some projects, but bad. Syntax consistency helps the brain
concentrate on the important things.

> At this point I am willing to be convinced either way, but I'll note the
> alternative:   we could also have a style entry that says that these
> (HAVE_NO and RETURN) macros are a common sight in Samba and these are
> the semantics.  I certainly got used to them pretty quickly. 

I am with Volker here. 'No surprises' beats 'slightly more convenient at
the time of writing' 10 - 1, for the simple reason that code is read
many more times than it is written. It is important for the code to be
readable, it is much more important than slight conveniences when you
write it.

Simo.

-- 
Simo Sorce
Samba Team GPL Compliance Officer <simo at samba.org>
Principal Software Engineer at Red Hat, Inc. <simo at redhat.com>