| Message ID | 20160708162803.GA29111@linux.intel.com (mailing list archive) |
|---|---|
| State | Accepted |
| Delegated to: | Herbert Xu |
| Headers | show |
On Fri, Jul 08, 2016 at 09:28:03AM -0700, Tim Chen wrote: > > Sorry I was on vacation and didn't get to respond earlier. > Let's switch the above from | to || so the code logic is > clearer. Also clean up various multi-line comment style > inconsistencies in patch below. Nack. As I said the commenting style in the crypto API is the same as the network stack. So unless we decide to change both please stick to the current style. Cheers,
On Sat, 2016-07-09 at 00:45 +0800, Herbert Xu wrote: > On Fri, Jul 08, 2016 at 09:28:03AM -0700, Tim Chen wrote: > > > > > > Sorry I was on vacation and didn't get to respond earlier. > > Let's switch the above from | to || so the code logic is > > clearer. Also clean up various multi-line comment style > > inconsistencies in patch below. > Nack. As I said the commenting style in the crypto API is the > same as the network stack. So unless we decide to change both > please stick to the current style. > Will you like a patch with just the | to || change, or leave the code as is? Tim -- To unsubscribe from this list: send the line "unsubscribe linux-crypto" in the body of a message to majordomo@vger.kernel.org More majordomo info at http://vger.kernel.org/majordomo-info.html
[ rare comment rant. I think I'll do this once, and then ignore the discussion ] On Fri, Jul 8, 2016 at 9:45 AM, Herbert Xu <herbert@gondor.apana.org.au> wrote: > > Nack. As I said the commenting style in the crypto API is the > same as the network stack. So unless we decide to change both > please stick to the current style. Can we please get rid of the brain-damaged stupid networking comment syntax style, PLEASE? If the networking people cannot handle the pure awesomeness that is a balanced and symmetric traditional multi-line C style comments, then instead of the disgusting unbalanced crap that you guys use now, please just go all the way to the C++ mode. In other words, these three models are good: (a) /* This is a comment *./ (b) /* * This is also a comment, but it can now be cleanly * split over multiple lines */ (c) // This can be a single line. Or many. Your choice. and they are all obviously visually balanced. Sometimes you want (b) even for a single line, if you want the white-space to make it stand out more, but you can obviously do that with (c) too, by just surrounding it with two empty (comment) lines. The (c) form is particularly good for things like enum or structure member comments at the end of code, where you might want to align things up, but the ending comment marker ends up being visually pretty distracting (and lining _that_ up is too much make-believe work). There's also another acceptablr traditional multi-line style that you'll find in some places, but it's not the common kernel style: (d) /* This is an alternate multi-line format that isn't horrible, but not kernel style */ Note how all the above comment styles have a certain visual symmatry and balance. But no, the networking code picked *none* of the above sane formats. Instead, it picked these two models that are just half-arsed shit-for-brains: (no) /* This is disgusting drug-induced * crap, and should die */ (no-no-no) /* This is also very nasty * and visually unbalanced */ Please. The networking code actually has the *worst* possible comment style. You can literally find that (no-no-no) style, which is just really horribly disgusting and worse than the otherwise fairly similar (d) in pretty much every way. I'm not even going to start talking about the people who prefer to "box in" their comments, and line up both ends and have fancy boxes of stars around the whole thing. I'm sure that looks really nice if you are out of your mind on LSD, and have nothing better to do than to worry about the right alignment of the asterisks. I'd be happy to start moving the whole kernel over to the C++ style, it's been many many years since we had compatibility issues and we are all used to it by now, even if we weren't all fans originally. I really don't understand why the networking people think that their particularly ugly styles are fine. They are the most visually unbalanced version of _all_ the common comment styles, and have no actual advantages. So just get rid of the (no-no) and (no-no-no) forms. Not in one big go, but as people touch the code, just fix that mess up. Linus -- To unsubscribe from this list: send the line "unsubscribe linux-crypto" in the body of a message to majordomo@vger.kernel.org More majordomo info at http://vger.kernel.org/majordomo-info.html
Hi Linus, On Fri, Jul 8, 2016 at 7:19 PM, Linus Torvalds <torvalds@linux-foundation.org> wrote: > (c) > // This can be a single line. Or many. Your choice. > The (c) form is particularly good for things like enum or structure > member comments at the end of code, where you might want to align > things up, but the ending comment marker ends up being visually pretty > distracting (and lining _that_ up is too much make-believe work). While I'm a fan of the (c) form myself, I became used to not using it for kernel code. Except for internal comments that are not intended to be sent out. This works fine, as checkpatch will complain if I ever forget to remove them while preparing patches. The alternative would be to teach checkpatch to complain about FIXME, TODO, and XXX in comments... Gr{oetje,eeting}s, Geert -- Geert Uytterhoeven -- There's lots of Linux beyond ia32 -- geert@linux-m68k.org In personal conversations with technical people, I call myself a hacker. But when I'm talking to journalists I just say "programmer" or something like that. -- Linus Torvalds -- To unsubscribe from this list: send the line "unsubscribe linux-crypto" in the body of a message to majordomo@vger.kernel.org More majordomo info at http://vger.kernel.org/majordomo-info.html
On Fri, Jul 08, 2016 at 09:28:03AM -0700, Tim Chen wrote: > > From: Tim Chen <tim.c.chen@linux.intel.com> > Subject: [PATCH] crypto: Cleanup sha multi-buffer code to use || instead of | > for condition comparison and cleanup multiline comment style > > In sha*_ctx_mgr_submit, we currently use the | operator instead of || > ((ctx->partial_block_buffer_length) | (len < SHA1_BLOCK_SIZE)) > > Switching it to || and remove extraneous paranthesis to > adhere to coding style. > > Also cleanup inconsistent multiline comment style. > > Signed-off-by: Tim Chen <tim.c.chen@linux.intel.com> Patch applied. Thanks.
On Fri 2016-07-08 10:19:26, Linus Torvalds wrote: > [ rare comment rant. I think I'll do this once, and then ignore the discussion ] > > On Fri, Jul 8, 2016 at 9:45 AM, Herbert Xu <herbert@gondor.apana.org.au> wrote: > > > > Nack. As I said the commenting style in the crypto API is the > > same as the network stack. So unless we decide to change both > > please stick to the current style. > > Can we please get rid of the brain-damaged stupid networking comment > syntax style, PLEASE? Please? :-). Having different comment styles between networking and the rest is confusing. And yes, this uglyness is documented for net/ and drivers/net/, but not for crypto/, so at the very least Documentation/CodingStyle should be updated. Pavel
On Mon, 2016-07-18 at 10:59 +0200, Pavel Machek wrote: > On Fri 2016-07-08 10:19:26, Linus Torvalds wrote: > > > > [ rare comment rant. I think I'll do this once, and then ignore the discussion ] > > > > On Fri, Jul 8, 2016 at 9:45 AM, Herbert Xu <herbert@gondor.apana.org.au> wrote: > > > > > > > > > Nack. As I said the commenting style in the crypto API is the > > > same as the network stack. So unless we decide to change both > > > please stick to the current style. > > Can we please get rid of the brain-damaged stupid networking comment > > syntax style, PLEASE? > Please? :-). Having different comment styles between networking and > the rest is confusing. > > And yes, this uglyness is documented for net/ and drivers/net/, but > not for crypto/, so at the very least Documentation/CodingStyle should > be updated. > > Pavel The cleanup patch has already been merged. Thanks. Tim -- To unsubscribe from this list: send the line "unsubscribe linux-crypto" in the body of a message to majordomo@vger.kernel.org More majordomo info at http://vger.kernel.org/majordomo-info.html
diff --git a/arch/x86/crypto/sha1-mb/sha1_mb.c b/arch/x86/crypto/sha1-mb/sha1_mb.c index 561b286..9e5b671 100644 --- a/arch/x86/crypto/sha1-mb/sha1_mb.c +++ b/arch/x86/crypto/sha1-mb/sha1_mb.c @@ -304,7 +304,7 @@ static struct sha1_hash_ctx *sha1_ctx_mgr_submit(struct sha1_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 < SHA1_BLOCK_SIZE)) { + if (ctx->partial_block_buffer_length || len < SHA1_BLOCK_SIZE) { /* * Compute how many bytes to copy from user buffer into * extra block diff --git a/arch/x86/crypto/sha256-mb/sha256_mb.c b/arch/x86/crypto/sha256-mb/sha256_mb.c index c9d5dcc..89fa85e 100644 --- a/arch/x86/crypto/sha256-mb/sha256_mb.c +++ b/arch/x86/crypto/sha256-mb/sha256_mb.c @@ -283,7 +283,8 @@ static struct sha256_hash_ctx *sha256_ctx_mgr_submit(struct sha256_ctx_mgr *mgr, ctx->incoming_buffer = buffer; ctx->incoming_buffer_length = len; - /* Store the user's request flags and mark this ctx as currently + /* + * Store the user's request flags and mark this ctx as currently * being processed. */ ctx->status = (flags & HASH_LAST) ? @@ -299,8 +300,9 @@ 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)) { - /* Compute how many bytes to copy from user buffer into + if (ctx->partial_block_buffer_length || len < SHA256_BLOCK_SIZE) { + /* + * Compute how many bytes to copy from user buffer into * extra block */ uint32_t copy_len = SHA256_BLOCK_SIZE - @@ -323,7 +325,8 @@ static struct sha256_hash_ctx *sha256_ctx_mgr_submit(struct sha256_ctx_mgr *mgr, /* The extra block should never contain more than 1 block */ assert(ctx->partial_block_buffer_length <= SHA256_BLOCK_SIZE); - /* If the extra block buffer contains exactly 1 block, + /* + * If the extra block buffer contains exactly 1 block, * it can be hashed. */ if (ctx->partial_block_buffer_length >= SHA256_BLOCK_SIZE) { diff --git a/arch/x86/crypto/sha512-mb/sha512_mb.c b/arch/x86/crypto/sha512-mb/sha512_mb.c index 676f0f2..f4cf5b7 100644 --- a/arch/x86/crypto/sha512-mb/sha512_mb.c +++ b/arch/x86/crypto/sha512-mb/sha512_mb.c @@ -253,7 +253,8 @@ static struct sha512_hash_ctx int flags) { if (flags & (~HASH_ENTIRE)) { - /* User should not pass anything other than FIRST, UPDATE, or + /* + * User should not pass anything other than FIRST, UPDATE, or * LAST */ ctx->error = HASH_CTX_ERROR_INVALID_FLAGS; @@ -284,7 +285,8 @@ static struct sha512_hash_ctx ctx->partial_block_buffer_length = 0; } - /* If we made it here, there were no errors during this call to + /* + * If we made it here, there were no errors during this call to * submit */ ctx->error = HASH_CTX_ERROR_NONE; @@ -293,7 +295,8 @@ static struct sha512_hash_ctx ctx->incoming_buffer = buffer; ctx->incoming_buffer_length = len; - /* Store the user's request flags and mark this ctx as currently being + /* + * Store the user's request flags and mark this ctx as currently being * processed. */ ctx->status = (flags & HASH_LAST) ? @@ -309,7 +312,7 @@ static struct sha512_hash_ctx * 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 < SHA512_BLOCK_SIZE)) { + if (ctx->partial_block_buffer_length || len < SHA512_BLOCK_SIZE) { /* Compute how many bytes to copy from user buffer into extra * block */