pistacheio / pistache

A high-performance REST toolkit written in C++
https://pistacheio.github.io/pistache/
Apache License 2.0
3.12k stars 688 forks source link

fix: drop memset() calls #1139

Closed Tachi107 closed 1 year ago

Tachi107 commented 1 year ago

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:

codecov-commenter commented 1 year ago

Codecov Report

Patch coverage: 100.00% and project coverage change: -0.16 :warning:

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

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

:exclamation: 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 ```diff @@ 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](https://app.codecov.io/gh/pistacheio/pistache/pull/1139?src=pr&el=tree&utm_medium=referral&utm_source=github&utm_content=comment&utm_campaign=pr+comments&utm_term=None) | Coverage Δ | | |---|---|---| | [src/client/client.cc](https://app.codecov.io/gh/pistacheio/pistache/pull/1139?src=pr&el=tree&utm_medium=referral&utm_source=github&utm_content=comment&utm_campaign=pr+comments&utm_term=None#diff-c3JjL2NsaWVudC9jbGllbnQuY2M=) | `83.96% <100.00%> (-0.08%)` | :arrow_down: | | [src/common/mime.cc](https://app.codecov.io/gh/pistacheio/pistache/pull/1139?src=pr&el=tree&utm_medium=referral&utm_source=github&utm_content=comment&utm_campaign=pr+comments&utm_term=None#diff-c3JjL2NvbW1vbi9taW1lLmNj) | `90.04% <100.00%> (-0.05%)` | :arrow_down: | | [src/common/net.cc](https://app.codecov.io/gh/pistacheio/pistache/pull/1139?src=pr&el=tree&utm_medium=referral&utm_source=github&utm_content=comment&utm_campaign=pr+comments&utm_term=None#diff-c3JjL2NvbW1vbi9uZXQuY2M=) | `87.76% <100.00%> (+0.22%)` | :arrow_up: | | [src/server/listener.cc](https://app.codecov.io/gh/pistacheio/pistache/pull/1139?src=pr&el=tree&utm_medium=referral&utm_source=github&utm_content=comment&utm_campaign=pr+comments&utm_term=None#diff-c3JjL3NlcnZlci9saXN0ZW5lci5jYw==) | `70.94% <100.00%> (-0.17%)` | :arrow_down: | ... and [2 files with indirect coverage changes](https://app.codecov.io/gh/pistacheio/pistache/pull/1139/indirect-changes?src=pr&el=tree-more&utm_medium=referral&utm_source=github&utm_content=comment&utm_campaign=pr+comments&utm_term=None)

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

dennisjenkins75 commented 1 year ago

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

Tachi107 commented 1 year ago

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 commented 1 year ago

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 commented 1 year ago

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 commented 1 year ago

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)