Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

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

Closed
wants to merge 1 commit into from

Conversation

bagder
Copy link
Member

@bagder bagder commented May 11, 2018

... and unify the source code to adhere.

@monnerat
Copy link
Contributor

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

@bagder
Copy link
Member Author

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
Copy link
Contributor

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.

... and unify the source code to adhere.
@bagder
Copy link
Member Author

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
Copy link
Member Author

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
Copy link
Contributor

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
Copy link
Member Author

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
Copy link
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 21:22
@lock lock bot locked as resolved and limited conversation to collaborators Aug 19, 2018
Sign up for free to subscribe to this conversation on GitHub. Already have an account? Sign in.
Labels
None yet
Development

Successfully merging this pull request may close these issues.

None yet

3 participants