option to check duplicate object keys #801
Conversation
grisumbras
force-pushed
the
feature/check-duplicates
branch
from
eff5301
to
23f8e28
Compare
|
|
grisumbras
force-pushed
the
feature/check-duplicates
branch
from
057db37
to
6cc4182
Compare
grisumbras
force-pushed
the
feature/check-duplicates
branch
from
6cc4182
to
d05bc04
Compare
Codecov Report
Additional details and impacted files@@ Coverage Diff @@
## develop #801 +/- ##
========================================
Coverage 99.00% 99.00%
========================================
Files 71 71
Lines 6831 6833 +2
========================================
+ Hits 6763 6765 +2
Misses 68 68
Continue to review full report at Codecov.
|
|
yeah please don't merge this with the commit message as "different approach" :) |
include/boost/json/impl/object.ipp
Outdated
| @@ -232,6 +230,11 @@ object(detail::unchecked_object&& uo) | |||
| continue; | |||
| } | |||
| // handle duplicate | |||
| if( !uo.ignore_duplicate_keys() ) | |||
There was a problem hiding this comment.
This is a constant in the loop, so you should lift the conditional out of the loop
include/boost/json/impl/object.ipp
Outdated
| @@ -270,6 +274,11 @@ object(detail::unchecked_object&& uo) | |||
| } | |||
|
|
|||
| // handle duplicate | |||
| if( !uo.ignore_duplicate_keys() ) | |||
There was a problem hiding this comment.
This is a constant in the loop, so you should lift the conditional out of the loop
|
|
| void | ||
| push_object(std::size_t n); | ||
| error_code | ||
| push_object(std::size_t n, bool ignore_duplicates = true); |
There was a problem hiding this comment.
I think I prefer it as allow_duplicates, defaulting to false. Rationale: The default settings should have the all-zero bit-pattern. It is also easier to understand this way, you are opting in to the setting by assigning true. Rather than opting out to disallowing duplicates by assigning false.
There was a problem hiding this comment.
"Ignore duplicates" and "allow duplicates" are the same thing (at least in this case). The reverse of "ingore" would be "forbid". So, "forbid_duplicate_keys" option, with false as default?
include/boost/json/detail/value.hpp
Outdated
| template< class Object > | ||
| static constexpr | ||
| index_t | ||
| null_index(Object const&) noexcept |
There was a problem hiding this comment.
Why are we mucking about with value? And why are these templates?
| value* p = data_; | ||
| while(size_--) | ||
|
|
||
| for( value* p = data_; p != end_; p += 2 ) |
There was a problem hiding this comment.
Why did you change this loop? I prefer the while form, as it puts each of the statements on their own line. The for is wider, and puts multiple statements on one line, which makes debugging and code coverage worse.
There was a problem hiding this comment.
I see why, because the loop is simpler.
|
|
|
|
||
| std::size_t | ||
| unchecked_object:: | ||
| size() const noexcept |
There was a problem hiding this comment.
Did we move this from a header that wasn't visible to a header that is visible?
grisumbras
force-pushed
the
feature/check-duplicates
branch
from
f48cc9e
to
8715073
Compare
|
|
| } | ||
|
|
||
| inline |
There was a problem hiding this comment.
the inline goes on the definition not the declaration (my fault)
| @@ -70,6 +70,75 @@ find_in_object<string_view>( | |||
| object const& obj, | |||
| string_view key) noexcept; | |||
|
|
|||
| template< bool SmallTable, bool IgnoreDuplicates > | |||
| void init_from_unchecked( object& obj, unchecked_object& uo ) | |||
| void init_from_unchecked( object& obj, unchecked_object& uo ) | ||
| { | ||
| // insert all elements, keeping | ||
| // the last of any duplicate keys, unless IgnoreDuplicates is false. |
There was a problem hiding this comment.
There should be an assert here, the precondition of the function requires capacity() >= uo.size()
| @@ -364,16 +364,25 @@ push_array(std::size_t n) | |||
| st_.exchange(std::move(ua)); | |||
| } | |||
|
|
|||
| void | |||
| error_code | |||
There was a problem hiding this comment.
this is going to force the construction of a new error code for every object even when there is no error and even when ignore_duplicates is true. It should be an outparam. Or even a bool
| if( uo.size() ) | ||
| { | ||
| BOOST_JSON_FAIL( ec, error::duplicate_key ); | ||
| } |
There was a problem hiding this comment.
I believe that the way this is implemented has impacted performance, which is causing the regression in the per-commit performance checker.
Fix #127