[Top][All Lists]

[Date Prev][Date Next][Thread Prev][Thread Next][Date Index][Thread Index]

Re: [PATCH 1/2] stat-macros: Enhance to provide block-related informatio

From: Paul Eggert
Subject: Re: [PATCH 1/2] stat-macros: Enhance to provide block-related information.
Date: Tue, 07 Jun 2011 17:43:14 -0700
User-agent: Mozilla/5.0 (X11; U; Linux i686; en-US; rv: Gecko/20110516 Thunderbird/3.1.10

It'd be good to add something like this, thanks.

A few comments:

The time-related stat macros are already broken out into
stat-time.h.   Perhaps these size-related macros should
be broken out into stat-size.h?  That way, we could
leave stat-macros.h alone.  Many programs that need
CHMOD_MODE_BITS (in the current stat-macros.h) don't need
the heavy artillery used to configure the size-related macros.
If we make this change, the module should be renamed to
"stat-size" of course.

The indenting for the '#' lines in stat-macros.h isn't
consistent.  The usual gnulib style is to not count
"#ifndef STAT_MACROS_H" as a separate indenting level.

> +#  define ST_NBLOCKS(statbuf) \
> +  (S_ISREG ((statbuf).st_mode) \
> +   || S_ISDIR ((statbuf).st_mode) \
> +   ? st_blocks ((statbuf).st_size) : 0)

This indenting doesn't look right, as || and ? should be
different levels.  I'd combine the 2nd and 3rd lines into one line.
There's another definition of ST_NBLOCKS that has the same problem.
(I realize this problem is in coreutils but we might as well fix it....)

> +#   endif /* _CRAY */
> +#  endif /* not AIX PS/2 */
> +# endif /* !hpux */

I'd omit comments on the "else" and "endif" lines, such as
these, as they make the code harder to read.  They can be
helpful for long #if blocks, but these are short.

> +  AC_CHECK_HEADERS([sys/param.h])

>  Depends-on:
> +sys_stat
> +stdint

We can break the dependency on the stdint module
by using (size_t) -1 instead of SIZE_MAX.

Why does stat-macros need to depend on sys_stat?
Unless there's a specific need for the dependency,
I'd remove it.

reply via email to

[Prev in Thread] Current Thread [Next in Thread]