|| 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 <[email protected]>
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
*/
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 <[email protected]>
>
> 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
On Wed, Jun 29, 2016 at 10:05:53AM -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 <[email protected]>
> >
> > 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.
I know how the rules work. I just thought it looked more like a typo
than an optimization. It's normally a typo. It's hard to tell the
intent.
I think I'll modify my static checker to ignore these since the typo is
harmless.
regards,
dan carpenter
On Thu, 2016-06-30 at 10:50 +0300, Dan Carpenter wrote:
> On Wed, Jun 29, 2016 at 10:05:53AM -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 <[email protected]>
> > >
> > > diff --git 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.
> I know how the rules work.??I just thought it looked more like a typo
> than an optimization.??It's normally a typo.??It's hard to tell the
> intent.
The compiler could potentially emit the same code when
optimizing but at least gcc 5.3 doesn't.
It's probably useful to add a comment for the specific intent
here rather than change a potentially useful static checker.
Am 30.06.2016 13:16, schrieb Joe Perches:
> On Thu, 2016-06-30 at 10:50 +0300, Dan Carpenter wrote:
>> On Wed, Jun 29, 2016 at 10:05:53AM -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 <[email protected]>
>>>>
>>>> diff --git 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.
>> I know how the rules work. I just thought it looked more like a typo
>> than an optimization. It's normally a typo. It's hard to tell the
>> intent.
>
> The compiler could potentially emit the same code when
> optimizing but at least gcc 5.3 doesn't.
>
> It's probably useful to add a comment for the specific intent
> here rather than change a potentially useful static checker.
>
perhaps we can agree not to play tricks with a compiler.
Everything may be true for a certain version of CC but the next compiler is different.
just my 2 cents,
wh
The difference between | and || is that || has ordering constraints.
It's from the C standard, and not the compiler version.
regards,
dan carpenter
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 <[email protected]>
> >
> > 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. Is there any scenario where things may
break with the compiler?
Tim
On Thu, Jun 30, 2016 at 01:42:19PM -0700, 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 <[email protected]>
> > >
> > > 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. ?Is there any scenario where things may
> break with the compiler?
No. I'm going to remove the warning from the static checker like I said
earlier. It should only complain for && vs & typos, || vs | is
harmless.
regards,
dan carpenter
* Tim Chen <[email protected]> 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 <[email protected]>
> > >
> > > 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
On Fri, Jul 01, 2016 at 09:55:59AM +0200, Ingo Molnar wrote:
>
> Plus:
>
> > > > ? /* Compute how many bytes to copy from user buffer into
> > > > ? ?* extra block
> > > > ? ?*/
>
> please use the customary (multi-line) comment style:
This is the customary comment style of the networking stack and
the crypto API. So please don't change it.
Thanks,
--
Email: Herbert Xu <[email protected]>
Home Page: http://gondor.apana.org.au/~herbert/
PGP Key: http://gondor.apana.org.au/~herbert/pubkey.txt
* Herbert Xu <[email protected]> wrote:
> On Fri, Jul 01, 2016 at 09:55:59AM +0200, Ingo Molnar wrote:
> >
> > Plus:
> >
> > > > > ? /* Compute how many bytes to copy from user buffer into
> > > > > ? ?* extra block
> > > > > ? ?*/
> >
> > please use the customary (multi-line) comment style:
>
> This is the customary comment style of the networking stack and
> the crypto API. So please don't change it.
Guys, do you even read your own code??
That 'standard' is not being enforced consistently at all. Even in this very
series there's an example of that weird comment not being followed:
+++ 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,
/*
* Compute how many bytes to copy from user buffer into
* extra block
See how this comment block uses the standard coding style, while the next patch
has this weird coding style:
- 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
*/
The networking code's "exceptionalism" regarding the standard comment style is
super distracting and in this particular example it resulted in:
- inconsistent comment styles next to each other,
- the questionable '|' pattern hiding right next to:
- pointless parantheses around the (ctx->partial_block_buffer_length),
- which field name is also a misnomer.
So anyone doing security review of that weird '|' pattern first has to figure out
whether the 4 ugly code patterns amount to a security problem or not...
One thing that is more harmful that any of the coding styles: the inconsistent
coding style used by this code.
Btw., as a historic reference, there is nothing sacred about the 'networking
comments coding style': I was there (way too many years ago) when that comment
style was introduced by Alan Cox's first TCP/IP code drop, and it was little more
than just a random inconsistency that people are now treating as gospel...
Thanks,
Ingo
On Fri, Jul 01, 2016 at 12:13:30PM +0200, Ingo Molnar wrote:
>
> * Herbert Xu <[email protected]> wrote:
>
> > On Fri, Jul 01, 2016 at 09:55:59AM +0200, Ingo Molnar wrote:
> > >
> > > Plus:
> > >
> > > > > > ? /* Compute how many bytes to copy from user buffer into
> > > > > > ? ?* extra block
> > > > > > ? ?*/
> > >
> > > please use the customary (multi-line) comment style:
> >
> > This is the customary comment style of the networking stack and
> > the crypto API. So please don't change it.
>
> Guys, do you even read your own code??
>
> That 'standard' is not being enforced consistently at all. Even in this very
> series there's an example of that weird comment not being followed:
>
> +++ 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,
> /*
> * Compute how many bytes to copy from user buffer into
> * extra block
>
> See how this comment block uses the standard coding style, while the next patch
> has this weird coding style:
>
> - if ((ctx->partial_block_buffer_length) | (len < SHA256_BLOCK_SIZE)) {
> + if ((ctx->partial_block_buffer_length) || (len < SHA256_BLOCK_SIZE)) {
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.
Thanks.
Tim
---
From: Tim Chen <[email protected]>
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 <[email protected]>
---
arch/x86/crypto/sha1-mb/sha1_mb.c | 2 +-
arch/x86/crypto/sha256-mb/sha256_mb.c | 11 +++++++----
arch/x86/crypto/sha512-mb/sha512_mb.c | 11 +++++++----
3 files changed, 15 insertions(+), 9 deletions(-)
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
*/
--
2.5.5
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,
--
Email: Herbert Xu <[email protected]>
Home Page: http://gondor.apana.org.au/~herbert/
PGP Key: http://gondor.apana.org.au/~herbert/pubkey.txt
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
[ 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 <[email protected]> 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
Hi Linus,
On Fri, Jul 8, 2016 at 7:19 PM, Linus Torvalds
<[email protected]> 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 -- [email protected]
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
On Fri, Jul 08, 2016 at 09:28:03AM -0700, Tim Chen wrote:
>
> From: Tim Chen <[email protected]>
> 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 <[email protected]>
Patch applied. Thanks.
--
Email: Herbert Xu <[email protected]>
Home Page: http://gondor.apana.org.au/~herbert/
PGP Key: http://gondor.apana.org.au/~herbert/pubkey.txt
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 <[email protected]> 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
--
(english) http://www.livejournal.com/~pavelmachek
(cesky, pictures) http://atrey.karlin.mff.cuni.cz/~pavel/picture/horses/blog.html
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 <[email protected]> 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