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

fix: drop memset() calls #1139

Merged
merged 1 commit into from
Jun 27, 2023
Merged

fix: drop memset() calls #1139

merged 1 commit into from
Jun 27, 2023

Conversation

Tachi107
Copy link
Member

Using memset() to zero-initialize variables is kinda ugly, and not always correct. Use instead C++ and C23's empty brace list, which correctly zero-initializes any kind of object.

See the following docs for reference:

Using memset() to zero-initialize variables is kinda ugly, and not
always correct. Use instead C++ and C23's empty brace list, which
correctly zero-initializes any kind of object.

See the following docs for reference:

- <https://thephd.dev/ever-closer-c23-improvements#consistent-warningless-and-intuitive-initialization-with-->
- <https://www.open-std.org/jtc1/sc22/wg14/www/docs/n2900.htm>
@codecov-commenter
Copy link

codecov-commenter commented Jun 27, 2023

Codecov Report

Patch coverage: 100.00% and project coverage change: -0.16 ⚠️

Comparison is base (e884d6d) 78.60% compared to head (14dce46) 78.44%.

❗ Current head 14dce46 differs from pull request most recent head 6d9a0af. Consider uploading reports for the commit 6d9a0af to get more accurate results

❗ Your organization is not using the GitHub App Integration. As a result you may experience degraded service beginning May 15th. Please install the Github App Integration for your organization. Read more.

Additional details and impacted files
@@            Coverage Diff             @@
##           master    #1139      +/-   ##
==========================================
- Coverage   78.60%   78.44%   -0.16%     
==========================================
  Files          53       53              
  Lines        6884     6875       -9     
==========================================
- Hits         5411     5393      -18     
- Misses       1473     1482       +9     
Impacted Files Coverage Δ
src/client/client.cc 83.96% <100.00%> (-0.08%) ⬇️
src/common/mime.cc 90.04% <100.00%> (-0.05%) ⬇️
src/common/net.cc 87.76% <100.00%> (+0.22%) ⬆️
src/server/listener.cc 70.94% <100.00%> (-0.17%) ⬇️

... and 2 files with indirect coverage changes

☔ View full report in Codecov by Sentry.
📢 Do you have feedback about the report comment? Let us know in this issue.

@dennisjenkins75
Copy link
Collaborator

LGTM.. Except, personally, I prefer that "sizeof" always be treated (syntactically) like a function. It should take its arg in parentheses.

@Tachi107
Copy link
Member Author

Did I add new sizeofs? (I'm on my phone right now).

In any case, it seems that using sizeof(type or expression) instead of sizeof expression is not always correct according to my second link (paragraph "memset is Easy to Make Mistakes With"). I don't fully understand why though. I do prefer using the version without parenthesis, but I didn't think it was really that different.

@dennisjenkins75
Copy link
Collaborator

Parenthesis are REQUIRED for sizeof on a TYPE, but OPTIONAL for an expression. However, the general consensus (and my personal preference) is to ALWAYS use them. However, I am not as active in pistache development as I once was, so I defer to your judgement.

@kiplingw
Copy link
Member

I think for consistency sake, it's a good idea to use parentheses with sizeof. As @dennisjenkins75 pointed out, sometimes it's actually required.

@Tachi107
Copy link
Member Author

Tachi107 commented Jun 27, 2023

To be clear, I did not add any new sizeof expression uses - I actually removed one. Forget what I said in my previous comment, I misread the example.

Anyway, unless there are other feedbacks, this can be merged (the commitlint failure is irrelevant)

@kiplingw kiplingw merged commit 17644dd into master Jun 27, 2023
39 of 40 checks passed
@Tachi107 Tachi107 deleted the no-memset branch June 28, 2023 09:19
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
Development

Successfully merging this pull request may close these issues.

None yet

4 participants