Implement new string interface #37
Conversation
sdkrystian
force-pushed
the
new-string-interface
branch
from
a6960de to
55c1c5c
Compare
Codecov Report
@@ Coverage Diff @@
## develop #37 +/- ##
==========================================
- Coverage 99.08% 98.2% -0.88%
==========================================
Files 57 57
Lines 4823 4856 +33
==========================================
- Hits 4779 4769 -10
- Misses 44 87 +43
Continue to review full report at Codecov.
|
sdkrystian
force-pushed
the
new-string-interface
branch
2 times, most recently
from
e930cb2 to
6e9a145
Compare
sdkrystian
force-pushed
the
new-string-interface
branch
from
6e9a145 to
834b1e2
Compare
|
|
||
| #include <boost/json/error.hpp> | ||
| #include <boost/json/detail/string_impl.hpp> | ||
| #include <boost/json/detail/impl/string_impl.hpp> |
There was a problem hiding this comment.
I don't think you should be including this here, it should be included in <boost/json/detail/string_impl.hpp>. These directives:
https://github.com/sdkrystian/json/blob/834b1e29f463fc391652eb22ffbaacf13540ea74/include/boost/json/detail/string_impl.hpp#L292
Should read:
#include <boost/json/detail/impl/string_impl.hpp>
#ifdef BOOST_JSON_HEADER_ONLY
#include <boost/json/detail/impl/string_impl.ipp>
#endif
There was a problem hiding this comment.
Fixed; ptr_in_range was turned into an inline non-template function and moved into detail/string_impl.hpp
include/boost/json/impl/string.hpp
Outdated
| } | ||
|
|
||
| // KRYSTIAN TODO: this can be done without copies when | ||
| // reallocation is not needed. |
There was a problem hiding this comment.
Fix the comment to explain that we should use a separate algorithm if iterator category is Forward or better
include/boost/json/impl/string.hpp
Outdated
| iter_cat<InputIt>{}); | ||
| cleanup c{tmp, dsp}; | ||
| traits_type::copy( | ||
| impl_.replace_unchecked(first - begin(), last - first, tmp.size(), sp_), |
There was a problem hiding this comment.
long line.. out of balance with the other lines and blocks the chi. consider:
traits_type::copy(
impl_.replace_unchecked(
first - begin(),
last - first,
tmp.size(),
sp_),
tmp.data(),
tmp.size());
return *this;
There was a problem hiding this comment.
Moved to multiple lines.
| BOOST_TEST(s == std::string( | ||
| t.v1).insert(1, t.s2, 1, 3)); | ||
| }); | ||
| //// insert(size_type, string const&, size_type, size_type) |
There was a problem hiding this comment.
need a comment explaining why this is disabled, put your name in caps // SDK or something like that so the reader knows it is editorial.
test/string.cpp
Outdated
| { | ||
| std::string s1(t.v2.data(), t.v2.size()); | ||
| string s2(t.v2, sp); | ||
| BOOST_TEST(s2.replace(s2.begin(), s2.begin() + 1, t.v2.substr(0)) == s1.replace(0, 1, t.v2.data(), t.v2.size())); |
There was a problem hiding this comment.
Reformatted:
BOOST_TEST(
s2.replace(
s2.begin(),
s2.begin() + 1,
t.v2.substr(0)) ==
s1.replace(
0, 1,
t.v2.data(),
t.v2.size()));
Now that I can read it... we are using the iterator overload of json::string but the index overload of std::string? Why?
There was a problem hiding this comment.
Reformatted in new commit.
Now that I can read it... we are using the iterator overload of
json::stringbut the index overload ofstd::string? Why?
They perform the same operation, it was just easier to copy the index based tests.
|
Looks great, minor style nits, but the tests need some work. |
sdkrystian
force-pushed
the
new-string-interface
branch
2 times, most recently
from
0c8384a to
6027ce3
Compare
sdkrystian
force-pushed
the
new-string-interface
branch
from
42b0f37 to
7552858
Compare
2 participants
This implements the new
boost::json::stringinterface, along with checked replacement and insertion.replaceandinsertforInputIteratorstill need to be implemented as checked.