From mboxrd@z Thu Jan 1 00:00:00 1970 Return-Path: Received: (majordomo@vger.kernel.org) by vger.kernel.org via listexpand id S1751863AbcGAH4H (ORCPT ); Fri, 1 Jul 2016 03:56:07 -0400 Received: from mail-wm0-f65.google.com ([74.125.82.65]:35009 "EHLO mail-wm0-f65.google.com" rhost-flags-OK-OK-OK-OK) by vger.kernel.org with ESMTP id S1751036AbcGAH4E (ORCPT ); Fri, 1 Jul 2016 03:56:04 -0400 Date: Fri, 1 Jul 2016 09:55:59 +0200 From: Ingo Molnar To: Tim Chen Cc: "H. Peter Anvin" , Dan Carpenter , Herbert Xu , "David S. Miller" , Thomas Gleixner , Ingo Molnar , x86@kernel.org, Megha Dey , "Wang, Rui Y" , Denys Vlasenko , Xiaodong Liu , linux-crypto@vger.kernel.org, linux-kernel@vger.kernel.org, kernel-janitors@vger.kernel.org Subject: Re: [patch] crypto: sha256-mb - cleanup a || vs | typo Message-ID: <20160701075559.GA11902@gmail.com> References: <20160629144242.GE22818@mwanda> <8538242a-eab7-127e-e47e-26027fee4f6d@zytor.com> <1467319339.22178.256.camel@linux.intel.com> MIME-Version: 1.0 Content-Type: text/plain; charset=iso-8859-1 Content-Disposition: inline Content-Transfer-Encoding: 8bit In-Reply-To: <1467319339.22178.256.camel@linux.intel.com> User-Agent: Mutt/1.5.24 (2015-08-30) Sender: linux-kernel-owner@vger.kernel.org List-ID: X-Mailing-List: linux-kernel@vger.kernel.org * Tim Chen wrote: > On Wed, 2016-06-29 at 10:05 -0700, H. Peter Anvin wrote: > > On 06/29/16 07:42, Dan Carpenter wrote: > > > > > > > > > > > > > > > > > and | behave basically the same here but || is intended.  It causes a > > > static checker warning to mix up bitwise and logical operations. > > > > > > Signed-off-by: Dan Carpenter > > > > > > diff --git a/arch/x86/crypto/sha256-mb/sha256_mb.c b/arch/x86/crypto/sha256-mb/sha256_mb.c > > > index c9d5dcc..4ec895a 100644 > > > --- a/arch/x86/crypto/sha256-mb/sha256_mb.c > > > +++ b/arch/x86/crypto/sha256-mb/sha256_mb.c > > > @@ -299,7 +299,7 @@ static struct sha256_hash_ctx *sha256_ctx_mgr_submit(struct sha256_ctx_mgr *mgr, > > >    * Or if the user's buffer contains less than a whole block, > > >    * append as much as possible to the extra block. > > >    */ > > > - if ((ctx->partial_block_buffer_length) | (len < SHA256_BLOCK_SIZE)) { > > > + if ((ctx->partial_block_buffer_length) || (len < SHA256_BLOCK_SIZE)) { > > >   /* Compute how many bytes to copy from user buffer into > > >    * extra block > > >    */ > > > > > As far as I know the | was an intentional optimization, so you may way > > to look at the generated code. > > > > -hpa > > > > Yes, this is an intentional optimization. [...] Please don't do intentional optimizations while mixing them with a very ugly coding style: if ((ctx->partial_block_buffer_length) | (len < SHA256_BLOCK_SIZE)) { The extra, unnecessary parantheses around ctx->partial_block_buffer_length will make the ordinary reader assume that the person who wrote the code was unsure about basic C syntax details and typoed the '|' as well ... Also, for heaven's (and readability's) sake, pick shorter structure field names. What's wrong with ctx->partial_block_buf_len? Also, even if the '|' was intentional - wouldn't it result in better code to use '||'? Plus: > > >   /* Compute how many bytes to copy from user buffer into > > >    * extra block > > >    */ please use the customary (multi-line) comment style: /* * Comment ..... * ...... goes here. */ specified in Documentation/CodingStyle. Thanks, Ingo