[PATCH] #ifdef code cleanup

Andrew Bartlett abartlet at samba.org
Sat Nov 24 17:11:34 UTC 2018


On Sat, 2018-11-24 at 10:58 +0100, Ralph Böhme wrote:
> On Sat, Nov 24, 2018 at 10:42:05AM +1300, Andrew Bartlett via samba-technical wrote:
> > On Fri, 2018-11-23 at 15:33 +0100, Andreas Schneider wrote:
> > > On Friday, 23 November 2018 09:25:23 CET Andrew Bartlett via samba-technical
> > > wrote:
> > > > On Fri, 2018-11-23 at 10:06 +1300, Andrew Bartlett via samba-technical
> > > > 
> > > > wrote:
> > > > > On Thu, 2018-11-22 at 21:32 +0100, Andreas Schneider wrote:
> > > > > > On Thursday, 22 November 2018 19:36:57 CET Andrew Bartlett via
> > > > > > samba-technical> >
> > > > > > wrote:
> > > > > > > 
> > > > > > > This looks good, but honestly I'm a bit nervous.  Any chance you have
> > > > > > > done a build somehow forcing -E into the gcc line and compared the
> > > > > > > results before/after?
> > > > > > 
> > > > > > This is mostly from CFLAGS="-Wundef"
> > > > > 
> > > > > Sure, it's not the detection method I'm worried about, it is that large
> > > > > patch sets like this can easily slip in typo or similar and cause the
> > > > > condition to no longer match.
> > > > > 
> > > > > A way to guard against that is to demonstrate that the build produces
> > > > > the same output with and without the patch.
> > > > > 
> > > > > When I make big changes to our selftest system I ensure that the same
> > > > > tests (or if different, I can account for the difference) still get
> > > > > printed in the output stream, to ensure we don't silently miss tests.
> > > > > 
> > > > > Likewise I'm pretty sure I did checking of the config.h results during
> > > > > the waf upgrade.
> > > > > 
> > > > > Perhaps build with eg CC="my-cc.sh" that compiles with gcc -E and then
> > > > > again with a full gcc, to get at the CPP output?
> > > > 
> > > > I tried and failed to build that way with the attached.  No idea why it
> > > > doesn't work.  So this isn't a reasonable request.  If someone else can
> > > > eyeball the whole series and check it is correct, I'll be happy.
> > > 
> > > Here is a list produced with the attach script. I've added comments where they
> > > are checked, but the check fails so do not show up at all in config.h or we
> > > have #ifdef in the code and no configure check for it, which means it is dead
> > > code.
> > 
> > I did a test on my host with the script I suggested last night, fixed
> > up to finally work.
> > 
> > I had to revert:
> > 
> > s3: Remove unsused MMAP_BLACKLIST ifdef checks
> > lib:replace: Check if HAVE_DECL_ENVIRON is defined first
> > 
> > (these can be reasoned about independently).
> > 
> > To make it give the (almost) identical results you see below.
> 
> the gcc -E trick is really nice. Can we put that somewhere in the wiki? If you 
> have a suggestion where to put it, I'll happily volunteer to make the change. :)

Thanks!  It certainly should go in the wiki, and I was also thinking to
put it in the source tree also just so we don't loose it (it took quite
a few rounds to get it to work).  

Perhaps a new page on 'testing build and selftest changes', explaining
about checking config.h for differences, checking the sum of the
autobuild output (I'll find the horrible grep I used to see the list of
changed test), and checking this.

Also, we should also retrospectively run it over the big waf change and
see if shows us anything (before we find out the hard way after the
release...).

Andrew Bartlett

-- 
Andrew Bartlett                       http://samba.org/~abartlet/
Authentication Developer, Samba Team  http://samba.org
Samba Developer, Catalyst IT          http://catalyst.net.nz/services/samba





More information about the samba-technical mailing list