[Date Prev][Date Next][Thread Prev][Thread Next][Date Index][Thread Index]
Re: [PATCH 1/2] stat-macros: Enhance to provide block-related informatio
Re: [PATCH 1/2] stat-macros: Enhance to provide block-related information.
Tue, 07 Jun 2011 17:43:14 -0700
Mozilla/5.0 (X11; U; Linux i686; en-US; rv:126.96.36.199) 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 */
> +#endif /* HAVE_STRUCT_STAT_ST_BLOCKS */
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])
This should be AC_CHECK_HEADERS_ONCE.
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.