copied strings deprecation #71
Conversation
| char* set_prefix_impl(std::size_t); | ||
| struct prefix_holder { | ||
| message_base& mb_; | ||
| span<char> prefix_; |
There was a problem hiding this comment.
why span instead of string_view?
There was a problem hiding this comment.
span is used because we're going to be copying into this, i.e. it's mutable.
string_view should really only be used for const views.
src/message_base.cpp
Outdated
|
|
||
| message_base:: | ||
| prefix_holder:: | ||
| ~prefix_holder() { |
There was a problem hiding this comment.
why do the braces keep ending up on another line instead of their own line?
| char* buf_ = nullptr; | ||
| std::size_t n_ = 0; | ||
|
|
||
| prefix_holder(message_base& mb, std::size_t n); |
Codecov ReportAll modified and coverable lines are covered by tests ✅
Additional details and impacted files@@ Coverage Diff @@
## develop #71 +/- ##
===========================================
+ Coverage 87.84% 88.09% +0.25%
===========================================
Files 77 78 +1
Lines 4302 4260 -42
===========================================
- Hits 3779 3753 -26
+ Misses 523 507 -16
Continue to review full report in Codecov by Sentry.
|
|
GCOVR code coverage report https://71.http-proto.prtest.cppalliance.org/gcovr/index.html |
| dest, | ||
| ms.data(), | ||
| ms.size()); | ||
| dest += ms.size(); | ||
| *dest++ = ' '; | ||
| std::memcpy( | ||
| std::memmove( |
There was a problem hiding this comment.
are you sure memmove is needed instead of memcpy?
There was a problem hiding this comment.
Yup. This is because this method is invoked with self-aliasing string_views so in this case, it's correct to use memmove.
If you switch it to memcpy, the sanitizer builds will fail with an invalid memcpy violation.
cmazakas
force-pushed
the
feature/copied-strings-deprecation
branch
from
93499f7 to
5b6bd15
Compare
|
GCOVR code coverage report https://71.http-proto.prtest.cppalliance.org/gcovr/index.html |
cmazakas
force-pushed
the
feature/copied-strings-deprecation
branch
from
5b6bd15 to
59137db
Compare
|
GCOVR code coverage report https://71.http-proto.prtest.cppalliance.org/gcovr/index.html |
cmazakas
force-pushed
the
feature/copied-strings-deprecation
branch
from
59137db to
c799fb7
Compare
|
GCOVR code coverage report https://71.http-proto.prtest.cppalliance.org/gcovr/index.html |
|
|
||
| prefix_op( | ||
| message_base& mb, | ||
| std::size_t n) |
There was a problem hiding this comment.
We shouldn't apply noexcept here because this function allocates and it also enforces a length check via detail::throw_length_error().
src/detail/header.hpp
Outdated
| max_offset - h.size)) | ||
| detail::throw_length_error(); | ||
|
|
||
| auto n0 = detail::header::bytes_needed( |
There was a problem hiding this comment.
When I use short variable names I typically use, say "n0" to mean the old value and "n1" to mean the new value so you might think about changing this to n1.
There was a problem hiding this comment.
Makes sense. Good to know, thank you.
cmazakas
force-pushed
the
feature/copied-strings-deprecation
branch
from
c799fb7 to
337ddc6
Compare
|
GCOVR code coverage report https://71.http-proto.prtest.cppalliance.org/gcovr/index.html |
cmazakas
force-pushed
the
feature/copied-strings-deprecation
branch
from
337ddc6 to
a51b002
Compare
|
GCOVR code coverage report https://71.http-proto.prtest.cppalliance.org/gcovr/index.html |
|
This looks good |
Replace copied_strings with a dedicated helper that avoids the need for an extraneous copy. Also add an
align_up()helper to be used in a few other places in the codebase eventually.closes #59