clang-tidy and manual stuff #2177
Conversation
Codecov Report
@@ Coverage Diff @@
## main #2177 +/- ##
==========================================
- Coverage 63.30% 63.29% -0.02%
==========================================
Files 99 99
Lines 19596 19591 -5
Branches 9559 9556 -3
==========================================
- Hits 12406 12400 -6
- Misses 5116 5117 +1
Partials 2074 2074
Continue to review full report at Codecov.
|
| auto wrote = static_cast<size_t>(snprintf(temp, sizeof(temp), "%04d%02d%02d", date_.year, date_.month, date_.day)); | ||
| std::memcpy(buf, temp, wrote); |
There was a problem hiding this comment.
| auto wrote = static_cast<size_t>(snprintf(temp, sizeof(temp), "%04d%02d%02d", date_.year, date_.month, date_.day)); | |
| std::memcpy(buf, temp, wrote); | |
| auto wrote = snprintf(temp, sizeof(temp), "%04d%02d%02d", date_.year, date_.month, date_.day); | |
| assert(wrote >= 0); | |
| std::memcpy(buf, temp, static_cast<size_t>(wrote)); |
There was a problem hiding this comment.
That makes no sense. The function returns size_t.
There was a problem hiding this comment.
That's incorrect:
https://en.cppreference.com/w/c/io/fprintf
snprintf returns int, and it sometimes returns negative numbers on error. I don't think it will ever return a negative number here though, which is why I think an assert is sufficient, rather throwing an exception.
There was a problem hiding this comment.
I also noticed that snprintf returns int but I thought it would not ever happen in this case. We recently removed most (if not all) the asserts from the codebase since those only get executed in Debug builds.
For me the code it is fine as it is at the moment. At the end we need to place 1 static_assert somewhere and I do not see any advantage with having it in the call to snprintf compared to doing it later (line 848).
I am approving the PR but not merging it yet. @kevinbackhouse let us know if you really think there is a strong reason/advantage to choose your suggestion.
|
Removed last commit. |
neheb
force-pushed
the
1
branch
2 times, most recently
from
e7d567a
to
4ae033c
Compare
Signed-off-by: Rosen Penev <[email protected]>
Signed-off-by: Rosen Penev <[email protected]>
Signed-off-by: Rosen Penev <[email protected]>
Signed-off-by: Rosen Penev <[email protected]>
Signed-off-by: Rosen Penev <[email protected]>
Signed-off-by: Rosen Penev <[email protected]>
Signed-off-by: Rosen Penev <[email protected]>
Signed-off-by: Rosen Penev <[email protected]>
Signed-off-by: Rosen Penev <[email protected]>
Found with google-build-using-namespace Signed-off-by: Rosen Penev <[email protected]>
Signed-off-by: Rosen Penev <[email protected]>
Signed-off-by: Rosen Penev <[email protected]>
Signed-off-by: Rosen Penev <[email protected]>
| @@ -41,13 +43,6 @@ using DWORD = unsigned long; | |||
| static int WSAGetLastError() { | |||
| return errno; | |||
| } | |||
|
|
|||
| auto wrote = static_cast<size_t>(snprintf(temp, sizeof(temp), "%04d%02d%02d", date_.year, date_.month, date_.day)); | ||
| std::memcpy(buf, temp, wrote); |
There was a problem hiding this comment.
I also noticed that snprintf returns int but I thought it would not ever happen in this case. We recently removed most (if not all) the asserts from the codebase since those only get executed in Debug builds.
For me the code it is fine as it is at the moment. At the end we need to place 1 static_assert somewhere and I do not see any advantage with having it in the call to snprintf compared to doing it later (line 848).
I am approving the PR but not merging it yet. @kevinbackhouse let us know if you really think there is a strong reason/advantage to choose your suggestion.
No description provided.