Re: [lustre-devel] [PATCH 09/10] staging: lustre: libcfs: remove zero comparisons in headers

[Dilger, Andreas]

On Nov 18, 2016, at 09:48, James Simmons <jsimmons@xxxxxxxxxxxxx> wrote:
> 
> Remove the zero comparisions in the libcfs headers.
> 
> Signed-off-by: James Simmons <jsimmons@xxxxxxxxxxxxx>
> ---
> .../lustre/include/linux/libcfs/libcfs_crypto.h    |    2 +-
> .../lustre/include/linux/libcfs/libcfs_fail.h      |    4 +-
> .../lustre/include/linux/libcfs/libcfs_hash.h      |   32 ++++++++++----------
> 3 files changed, 19 insertions(+), 19 deletions(-)
> 
> diff --git a/drivers/staging/lustre/include/linux/libcfs/libcfs_crypto.h b/drivers/staging/lustre/include/linux/libcfs/libcfs_crypto.h
> index a0865e5..8f34c5d 100644
> --- a/drivers/staging/lustre/include/linux/libcfs/libcfs_crypto.h
> +++ b/drivers/staging/lustre/include/linux/libcfs/libcfs_crypto.h
> @@ -137,7 +137,7 @@ static inline unsigned char cfs_crypto_hash_alg(const char *algname)
> 	enum cfs_crypto_hash_alg hash_alg;
> 
> 	for (hash_alg = 0; hash_alg < CFS_HASH_ALG_MAX; hash_alg++)
> -		if (strcmp(hash_types[hash_alg].cht_name, algname) == 0)
> +		if (!strcmp(hash_types[hash_alg].cht_name, algname))

I'd really rather keep the "== 0" comparison for strcmp(), because IMHO
!strcmp() reads like "the strings do not compare the same" and is confusing.

> static inline int
> cfs_hash_is_iterating(struct cfs_hash *hs)
> {
> 	/* someone is calling cfs_hash_for_each_* */
> -	return hs->hs_iterating || hs->hs_iterators != 0;
> +	return hs->hs_iterating || hs->hs_iterators;

Likewise, I'd rather keep comparisons == 0 or != 0 for cases where the
variable is an actual number instead of just a return code or a pointer.

I don't think we need to be totally dogmatic about removing every comparison
with 0 in all of the code, as that can reduce readability in some cases.

I know this patch has already landed, and it isn't the end of the world
either way, but just wanted to forestall similar changes being made through
the rest of the code.

Cheers, Andreas