Re: [PATCH] test-exclude.c: remove unmatched #endif

[Jim Meyering]

Bruno Haible wrote:
> Hi Jim,
>
>> I was surprised to find that a test did not compile.
>> * tests/test-exclude.c: Remove stray #endif, left over from
>> the change of a week ago.
>
> Thanks for the fix; my mistake.
>
>> This one was interesting in that it arose (I suspect)
>> due to the fact that code upon which the offending change
>> was based had misleadingly-indented cpp directives.
>
> Yes, this was part of the problem. The other part was that blank lines
> normally help to find out the start and end of semantic blocks; in this
> case, the absence of blank lines misled me.
>
>> Easiest would be to convert
>> all files, regardless of owner; would anyone prefer I *not* do that?
>
> Yes, I am opposed to it. No way to have me agree to changing more than
> 50 files.

I admit that over 100 is a lot, and many are yours.
However, putting that in perspective, if we start only with .c files,
that'd be 119, which means over 80% of the *.c files already conform.

Would you mind if I adjust whichever of the 25 in tests/ that are yours ?

$ for i in $(cppi -l *.[ch]); do cppi $i > k; mv k $i;done
$ git diff . > k
$ diffstat < k
 test-cond.c           |   60 +++++++++---------
 test-duplocale.c      |   14 ++--
 test-fprintf-posix2.c |   24 +++----
 test-iconv.c          |    2
 test-isfinite.c       |   12 +--
 test-isinf.c          |   12 +--
 test-isnan.c          |    6 -
 test-isnand.h         |    2
 test-isnanf.h         |    2
 test-isnanl.h         |    2
 test-lock.c           |  160 +++++++++++++++++++++++++-------------------------
 test-mbmemcasecmp.h   |   36 +++++------
 test-mbsnrtowcs.c     |    2
 test-mbsrtowcs.c      |    2
 test-parse-datetime.c |    4 -
 test-poll.c           |   10 +--
 test-posix_spawn3.c   |    4 -
 test-printf-posix2.c  |   24 +++----
 test-quotearg.h       |   10 +--
 test-signbit.c        |   12 +--
 test-stdint.c         |    6 -
 test-tls.c            |   72 +++++++++++-----------
 test-tsearch.c        |    6 -
 test-wcsnrtombs.c     |    2
 test-wcsrtombs.c      |    2
 25 files changed, 244 insertions(+), 244 deletions(-)

> 10 files would be ok. In other words, the proposed change is too far
> away from current practice. I could agree to something that is closer
> to current practice, yet avoids some of the biggest pitfalls.
>
> The pitfall in this case was to have a block of lines, without blank lines
> in them, which had an unbalanced #if[def]/#endif count _and_ more than
> one #if[def]/#endif _and_ was not using cppi style.
>
> This is OK for me (does not present a pitfall):
>
>   #if A
>   #if B
>   foo
>   #endif
>   #endif
>
> This is OK too:
>
>   #if A
>   # if B
>
>   # endif
>
>   #endif
>
> This is OK too:
>
>   #if A
>
>   #if B
>
>   #endif
>
>   #endif

OK depends on your perspective and on the code.
When the distance between #if and matching #endif is too large,
it can be a big help to have consistent indentation.
In a 7-line example like the template above, it doesn't
make much of a difference.

Another advantage to religiously-consistent cpp directive indentation
is that you can easily write a grep-like tool (yes, I have one somewhere)
that will report the nested cpp conditions corresponding to each match.

Also, from a more mundane perspective,
it lets you distinguish conditional and unconditional
#include directives and #define directives based solely
on presence or absence of spaces after the "#".

> This is OK too:
>
>   #if A
>
>   #if B
>   foo
>   #endif
>
>   #endif
>
> But this is not OK:
>
>   #if A
>   #if B
>   foo
>   #endif
>
>   #endif
>
> Can cppi be extended (through a command-line option) to flag only the last 
> case as
> improperly indented?

Probably, if you're motivated.

> I haven't found a way to do so within 10 minutes, so I wrote a
> tool from scratch for doing this. It has some limitations / bugs (*), but on 
> the gnulib
> source code before the 2011-02-13 change it produces these warnings:
>
>   $ for f in `find lib -name '*.[hc]' | sort` `find tests -name '*.[hc]' | 
> sort`; do ./findmisindent $f; done
>   misindented block at lib/argp-namefrob.h:98
>   misindented block at lib/getopt_int.h:108
>   misindented block at lib/progreloc.c:144
>   misindented block at lib/progreloc.c:256
>   misindented block at lib/regexec.c:3971
>   misindented block at lib/vasnprintf.c:879
>   misindented block at lib/vasnprintf.c:4568
>   misindented block at tests/test-argmatch.c:29
>   misindented block at tests/test-exclude.c:63
>
> I.e. to fix this, we need to touch less than 10 files, and it catches to 
> pitfall
> in tests/test-exclude.c.
>
> (*) It does not have --help. It does not do argument checking. It uses 
> gets(). It does
>     not know about comment syntax. It does not know about preprocessor 
> directive
>     continuation lines.

One change currently induced by cppi that I'm not sure I like is this:

    diff --git a/tests/test-wcsnrtombs.c b/tests/test-wcsnrtombs.c
    index 4d49929..a6df532 100644
    --- a/tests/test-wcsnrtombs.c
    +++ b/tests/test-wcsnrtombs.c
    @@ -42,7 +42,7 @@ main (int argc, char *argv[])
           wchar_t input[10];
           size_t n;
           const wchar_t *src;
    -      #define BUFSIZE 20
    +#define BUFSIZE 20
           char buf[BUFSIZE];
           size_t ret;

I do see the aesthetic value in indenting the entire cpp directive,
but from a consistency standpoint, it is counterproductive.

I'm glad we have relatively few cpp directives, which means there
is much less value in consistent cpp indentation than in some other
situations.