Removing the NT_STATUS_HAVE_NO_MEMORY_AND_FREE macro

[Michael Adam]

On 2014-02-17 at 12:55 +1300, Andrew Bartlett wrote:
> On Sat, 2014-02-15 at 09:47 -0800, Jeremy Allison wrote:
> > On Sat, Feb 15, 2014 at 03:21:11PM +0100, Kamen Mazdrashki wrote:
> > > Hi list,
> > > 
> > > Perhaps I am missing something, but why we need this change to remove
> > > NT_STATUS_HAVE_NO_MEMORY_AND_FREE? I understand there is
> > > coding rule not to return from a macro, but in this case, this macro makes
> > > the code easier to read.
> > > 
> > > Take for instance: libgpo/gpo_util.c:gpo_copy() function.
> > > It is supposed to be just a simple copy-object implementation and using
> > > NT_STATUS_HAVE_NO_MEMORY_AND_FREE it look straightforward
> > > imho. But, now it is going to be bloated with 'if's all the way and this
> > > will
> > > literary double the length of it (which will make it hard to notice if you
> > > have
> > > missed to copy a struct field).
> > > 
> > > Just my 2c.
> > 
> > Well doing returns inside macros is generally bad
> > programming practice, especially when you may need
> > to do cleanups that get added later. Changes in
> > control flow really need to be explicit, not implicit
> > inside macros.
> > 
> > So it might make the code a little uglier, but it's
> > then easier to maintain in the long run.
> > 
> > My 2c (and why I +1'ed it :-).

I have used some of the code-flow-changin macros in the past
but am meanwhile convincet that they are absolutely a bad
practice and should be avoided. But...

> My starting position is that we should either have a coding style rule
> and follow it, or not have the rule.  You might recall I opposed this
> addition to the coding style - but not because I disagreed (I am
> persuaded, particularly in the cases where the macro does not say
> 'RETURN' or 'assert'), but because I feel that we should make an effort
> such as the one Garming has done to actually follow the new style at the
> point we set the rule. 

... our practice for coding rules is that we try to make
*new* code adhere to them. Existing code can be adapted
as it is touched. We do not usually walk over the existing
code and do changes just to make it adhere to the guidelines.
Doing this has several downsides:

- It blurs code ownership.
- It makes backports harder.
- It is seductive to do tons of scripted changes
  instead of concentrating on real work.

So as a consequence, while I do in principle agree
with the result of the patches, I am sceptical
towards this change.

Just my 2¢.

Michael

-------------- next part --------------
A non-text attachment was scrubbed...
Name: signature.asc
Type: application/pgp-signature
Size: 215 bytes
Desc: Digital signature
URL: <http://lists.samba.org/pipermail/samba-technical/attachments/20140217/e00398d9/attachment.pgp>