Re: [PATCH] di-set, ino-map: new modules, from coreutils

[Jim Meyering]

Bruno Haible wrote:
> Jim Meyering wrote:
>> * lib/ino-map.c: Likewise.
>> * lib/ino-map.h: Likewise.
>> * modules/ino-map: Likewise.
>> * modules/ino-map-tests: Likewise.
>> * tests/test-ino-map.c: Likewise.
>
> lib/ino-map.h could well use a copyright header.

It's so short and simple, adding a copyright header seems unwarranted.

> lib/ino-map.c looks 100% correct.
>
> For the tests, here's a suggested patch that addresses three issues:
>
>   - If ino-map.h were included right after config.h, it would be a test of its
>     self-containedness.
>
>   - ASSERT is now found in macros.h. The comment
>     /* FIXME: once/if in gnulib, use #include "macros.h" in place of this */
>     looks like you thought that this change was only necessary after moving
>     the module to gnulib. Not so: You can have a module that contains two
>     files
>       tests/test-ino-map.c
>       tests/macros.h
>     where the first file comes from coreutils and the second file comes from
>     gnulib. Absolutely no problem.

I've done that for the remaining definition of ASSERT in coreutils.

>   - Variable declaration after statement: a C99 feature.
>
>
> 2011-02-07  Bruno Haible  <address@hidden>
>
>       ino-map tests: refactor
>       * tests/test-ino-map.c: Include ino-map.h early. Include macros.h. Drop
>       unnecessary includes.
>       (ASSERT): Remove macro.
>       (main): Make C90 compliant by avoiding variable declaration after
>       statement.
>       * modules/ino-map-tests (Files): Add tests/macros.h.

Thank you for the review and patch.
Those changes look fine.