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.
