checksrc: make sure sizeof() is used *with* parentheses #2563

Closed
wants to merge 1 commit into
from

Conversation

Projects
None yet
3 participants
@bagder
Member

bagder commented May 11, 2018

... and unify the source code to adhere.

@monnerat

This comment has been minimized.

Show comment
Hide comment
@monnerat

monnerat May 12, 2018

Collaborator

Well... this has the drawback of not differencing sizeof variable from sizeof(type), which IMHO, improves readability and code understanding.

Collaborator

monnerat commented May 12, 2018

Well... this has the drawback of not differencing sizeof variable from sizeof(type), which IMHO, improves readability and code understanding.

@bagder

This comment has been minimized.

Show comment
Hide comment
@bagder

bagder May 13, 2018

Member

Personally I don't make any distinction between those two versions in my head and I'm not sure why we need to make that distinction in the code. I so much prefer the paren version, especially when used in expressions like sizeof int + foo which I always have to look at twice to understand while having the parentheses there makes it more obvious and easier to read.

But perhaps more importantly, I don't see how we use sizeof paren vs non-paren consistently in the code now. This cleanup is mostly a fix to make us consistently always use the paren version.

Member

bagder commented May 13, 2018

Personally I don't make any distinction between those two versions in my head and I'm not sure why we need to make that distinction in the code. I so much prefer the paren version, especially when used in expressions like sizeof int + foo which I always have to look at twice to understand while having the parentheses there makes it more obvious and easier to read.

But perhaps more importantly, I don't see how we use sizeof paren vs non-paren consistently in the code now. This cleanup is mostly a fix to make us consistently always use the paren version.

@monnerat

This comment has been minimized.

Show comment
Hide comment
@monnerat

monnerat May 13, 2018

Collaborator

sizeof int + foo

This is a bad example that would fail at compile time.
Assuming you wanted to say sizeof var + foo, this can be rewritten as foo + sizeof var, which is much more readable.

Anyway, this is just a matter of styles and as in any open-source project, there are as many as contributors !
My initial remark is my 2 cents and only reflects my opinion: in any case, you're the referee.

If this is a new rule, https://curl.haxx.se/dev/code-style.html should be updated to mention it.

Collaborator

monnerat commented May 13, 2018

sizeof int + foo

This is a bad example that would fail at compile time.
Assuming you wanted to say sizeof var + foo, this can be rewritten as foo + sizeof var, which is much more readable.

Anyway, this is just a matter of styles and as in any open-source project, there are as many as contributors !
My initial remark is my 2 cents and only reflects my opinion: in any case, you're the referee.

If this is a new rule, https://curl.haxx.se/dev/code-style.html should be updated to mention it.

checksrc: make sure sizeof() is used *with* parentheses
... and unify the source code to adhere.
@bagder

This comment has been minimized.

Show comment
Hide comment
@bagder

bagder May 14, 2018

Member

I'll wait a while for some more feedback. This isn't in any hurry. Rebased and pushed to fix the merge conflict.

Member

bagder commented May 14, 2018

I'll wait a while for some more feedback. This isn't in any hurry. Rebased and pushed to fix the merge conflict.

@bagder

This comment has been minimized.

Show comment
Hide comment
@bagder

bagder May 14, 2018

Member

This is a bad example that would fail at compile time.

No it wouldn't fail. We actually have exactly this line in the code right now:

sizeof st->buf - st->bufend

... which is one of the lines that made me want to clean this up. For some reason I find that much harder to read than:

sizeof(st->buf) - st->bufend

And allowing without paren in some situations but not in others were just too hard to make checksrc work with =)

Member

bagder commented May 14, 2018

This is a bad example that would fail at compile time.

No it wouldn't fail. We actually have exactly this line in the code right now:

sizeof st->buf - st->bufend

... which is one of the lines that made me want to clean this up. For some reason I find that much harder to read than:

sizeof(st->buf) - st->bufend

And allowing without paren in some situations but not in others were just too hard to make checksrc work with =)

@monnerat

This comment has been minimized.

Show comment
Hide comment
@monnerat

monnerat May 14, 2018

Collaborator

sizeof st->buf - st->bufend

This one is OK, because st->buf is an expression, but sizeof int is forbidden: int is a type, not an expression.

Proof:

aaa.c:3:24: error: expected expression before ‘int’
  printf("%u\n", sizeof int);
                        ^~~

sizeof is an operator, not a function. It is followed either by an expression that may, as any expression, be parenthesed, or a parenthesed type (like a cast). You cannot have a type without parentheses.

Not putting unneeded parenthesis is the emphasing I like to see in code, because it helps you knowing if you have to look for a variable or a typedef.

See https://bytes.com/topic/c/answers/543699-sizeof-without-parenthesis

Collaborator

monnerat commented May 14, 2018

sizeof st->buf - st->bufend

This one is OK, because st->buf is an expression, but sizeof int is forbidden: int is a type, not an expression.

Proof:

aaa.c:3:24: error: expected expression before ‘int’
  printf("%u\n", sizeof int);
                        ^~~

sizeof is an operator, not a function. It is followed either by an expression that may, as any expression, be parenthesed, or a parenthesed type (like a cast). You cannot have a type without parentheses.

Not putting unneeded parenthesis is the emphasing I like to see in code, because it helps you knowing if you have to look for a variable or a typedef.

See https://bytes.com/topic/c/answers/543699-sizeof-without-parenthesis

@bagder

This comment has been minimized.

Show comment
Hide comment
@bagder

bagder May 14, 2018

Member

Not putting unneeded parenthesis is the emphasing I like to see in code, because it helps you knowing if you have to look for a variable or a typedef.

Yes, you told. And I showed you a case using such a non-paren case that I think is inferior without parentheses. Clearly different tastes.

Member

bagder commented May 14, 2018

Not putting unneeded parenthesis is the emphasing I like to see in code, because it helps you knowing if you have to look for a variable or a typedef.

Yes, you told. And I showed you a case using such a non-paren case that I think is inferior without parentheses. Clearly different tastes.

@jay

This comment has been minimized.

Show comment
Hide comment
@jay

jay May 15, 2018

Member

I also sometimes do it without parentheses however I have no objection. I can see how having a space after the sizeof may require more cognitive processing to understand an expression.

Member

jay commented May 15, 2018

I also sometimes do it without parentheses however I have no objection. I can see how having a space after the sizeof may require more cognitive processing to understand an expression.

@bagder bagder closed this in cb529b7 May 21, 2018

@bagder bagder deleted the bagder/unify-sizeof branch May 21, 2018

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment