Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

option to check duplicate object keys #801

Draft
wants to merge 3 commits into
base: develop
Choose a base branch
from
Draft
Show file tree
Hide file tree
Changes from 2 commits
Commits
File filter

Filter by extension

Filter by extension

Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
1 change: 1 addition & 0 deletions include/boost/json/detail/handler.hpp
Original file line number Diff line number Diff line change
Expand Up @@ -35,6 +35,7 @@ struct handler
max_string_size = string::max_size();

value_stack st;
bool ignore_duplicate_keys = true;

template<class... Args>
explicit
Expand Down
20 changes: 10 additions & 10 deletions include/boost/json/detail/impl/handler.ipp
Original file line number Diff line number Diff line change
Expand Up @@ -51,10 +51,10 @@ bool
handler::
on_object_end(
std::size_t n,
error_code&)
error_code& ec)
{
st.push_object(n);
return true;
ec = st.push_object(n, ignore_duplicate_keys);
return !ec.failed();
}

bool
Expand Down Expand Up @@ -85,7 +85,7 @@ on_key_part(
st.push_chars(s);
return true;
}

bool
handler::
on_key(
Expand All @@ -96,12 +96,12 @@ on_key(
st.push_key(s);
return true;
}

bool
handler::
on_string_part(
string_view s,
std::size_t,
std::size_t,
error_code&)
{
st.push_chars(s);
Expand All @@ -112,7 +112,7 @@ bool
handler::
on_string(
string_view s,
std::size_t,
std::size_t,
error_code&)
{
st.push_string(s);
Expand All @@ -138,7 +138,7 @@ on_int64(
st.push_int64(i);
return true;
}

bool
handler::
on_uint64(
Expand All @@ -160,7 +160,7 @@ on_double(
st.push_double(d);
return true;
}

bool
handler::
on_bool(
Expand All @@ -187,7 +187,7 @@ on_comment_part(
{
return true;
}

bool
handler::
on_comment(
Expand Down
35 changes: 20 additions & 15 deletions include/boost/json/detail/object.hpp
Original file line number Diff line number Diff line change
Expand Up @@ -28,30 +28,29 @@ class unchecked_object
// first one is a string key,
// second one is the value.
value* data_;
std::size_t size_;
value* end_;
storage_ptr const& sp_;
bool ignore_duplicates_;

public:
inline
~unchecked_object();

inline
unchecked_object(
value* data,
std::size_t size, // # of kv-pairs
storage_ptr const& sp) noexcept
: data_(data)
, size_(size)
, sp_(sp)
{
}
storage_ptr const& sp,
bool ignore_duplicates) noexcept;

unchecked_object(
unchecked_object&& other) noexcept
: data_(other.data_)
, size_(other.size_)
, end_(other.end_)
, sp_(other.sp_)
, ignore_duplicates_(other.ignore_duplicates_)
{
other.data_ = nullptr;
other.data_ = other.end_ = nullptr;
}

storage_ptr const&
Expand All @@ -60,19 +59,25 @@ class unchecked_object
return sp_;
}

inline
std::size_t
size() const noexcept
size() const noexcept;

bool
ignore_duplicate_keys() const noexcept
{
return size_;
return ignore_duplicates_;
}

value*
release() noexcept
front() noexcept
{
auto const data = data_;
data_ = nullptr;
return data;
return data_;
}

inline
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

the inline goes on the definition not the declaration (my fault)

void
pop_front() noexcept;
};

template<class CharRange>
Expand Down
3 changes: 3 additions & 0 deletions include/boost/json/error.hpp
Original file line number Diff line number Diff line change
Expand Up @@ -66,6 +66,9 @@ enum class error
/// error occured when trying to read input
input_error,

/// duplicate object key
duplicate_key,

//
// generic errors
//
Expand Down
2 changes: 2 additions & 0 deletions include/boost/json/impl/error.ipp
Original file line number Diff line number Diff line change
Expand Up @@ -45,6 +45,7 @@ case error::array_too_large: return "array too large";
case error::key_too_large: return "key too large";
case error::string_too_large: return "string too large";
case error::input_error: return "input error";
case error::duplicate_key: return "duplicate key";

case error::exception: return "got exception";
case error::test_failure: return "test failure";
Expand Down Expand Up @@ -93,6 +94,7 @@ case error::array_too_large:
case error::key_too_large:
case error::string_too_large:
case error::input_error:
case error::duplicate_key:
return condition::parse_error;

case error::missing_slash:
Expand Down
34 changes: 29 additions & 5 deletions include/boost/json/impl/object.hpp
Original file line number Diff line number Diff line change
Expand Up @@ -518,19 +518,43 @@ namespace detail {
unchecked_object::
~unchecked_object()
{
if(! data_)
return;
if(sp_.is_not_shared_and_deallocate_is_trivial())
return;
value* p = data_;
while(size_--)

for( value* p = data_; p != end_; p += 2 )
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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.

Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I see why, because the loop is simpler.

{
p[0].~value();
p[1].~value();
p += 2;
}
}

unchecked_object::
unchecked_object(
value* data,
std::size_t size,
storage_ptr const& sp,
bool ignore_duplicates) noexcept
: data_(data)
, end_(data + 2 * size)
, sp_(sp)
, ignore_duplicates_(ignore_duplicates)
{
}

std::size_t
unchecked_object::
size() const noexcept
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Did we move this from a header that wasn't visible to a header that is visible?

{
return std::size_t(end_ - data_) / 2;
}

void
unchecked_object::
pop_front() noexcept
{
data_ += 2;
}

} // detail

BOOST_JSON_NS_END
Expand Down
29 changes: 18 additions & 11 deletions include/boost/json/impl/object.ipp
Original file line number Diff line number Diff line change
Expand Up @@ -197,7 +197,7 @@ destroy() noexcept
//----------------------------------------------------------

object::
object(detail::unchecked_object&& uo)
object(detail::unchecked_object& uo)
: sp_(uo.storage())
{
if(uo.size() == 0)
Expand All @@ -210,20 +210,18 @@ object(detail::unchecked_object&& uo)
uo.size() <= max_size());
t_ = table::allocate(
uo.size(), 0, sp_);
t_->size = 0;

// insert all elements, keeping
// the last of any duplicate keys.
// the last of any duplicate keys, unless uo.ignore_duplicates is false.
auto dest = begin();
auto src = uo.release();
auto const end = src + 2 * uo.size();
if(t_->is_small())
{
t_->size = 0;
while(src != end)
for( ; uo.size(); uo.pop_front() )
{
auto src = uo.front();
access::construct_key_value_pair(
dest, pilfer(src[0]), pilfer(src[1]));
src += 2;
auto result = detail::find_in_object(*this, dest->key());
if(! result.first)
{
Expand All @@ -232,6 +230,11 @@ object(detail::unchecked_object&& uo)
continue;
}
// handle duplicate
if( !uo.ignore_duplicate_keys() )
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

This is a constant in the loop, so you should lift the conditional out of the loop

{
dest->~key_value_pair();
return;
}
auto& v = *result.first;
// don't bother to check if
// storage deallocate is trivial
Expand All @@ -243,11 +246,11 @@ object(detail::unchecked_object&& uo)
}
return;
}
while(src != end)
for( ; uo.size() ; uo.pop_front() )
{
auto src = uo.front();
access::construct_key_value_pair(
dest, pilfer(src[0]), pilfer(src[1]));
src += 2;
auto& head = t_->bucket(dest->key());
auto i = head;
for(;;)
Expand All @@ -260,6 +263,7 @@ object(detail::unchecked_object&& uo)
head = static_cast<index_t>(
dest - begin());
++dest;
++t_->size;
break;
}
auto& v = (*t_)[i];
Expand All @@ -270,6 +274,11 @@ object(detail::unchecked_object&& uo)
}

// handle duplicate
if( !uo.ignore_duplicate_keys() )
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

This is a constant in the loop, so you should lift the conditional out of the loop

{
dest->~key_value_pair();
return;
}
access::next(*dest) =
access::next(v);
// don't bother to check if
Expand All @@ -282,8 +291,6 @@ object(detail::unchecked_object&& uo)
break;
}
}
t_->size = static_cast<
index_t>(dest - begin());
}

object::
Expand Down
11 changes: 5 additions & 6 deletions include/boost/json/impl/parser.ipp
Original file line number Diff line number Diff line change
Expand Up @@ -32,20 +32,19 @@ parser(
size)
{
reset();
p_.handler().ignore_duplicate_keys = opt.ignore_duplicate_keys;
}

parser::
parser(
storage_ptr sp,
parse_options const& opt) noexcept
: p_(
opt,
: parser(
std::move(sp),
nullptr,
opt,
static_cast<unsigned char*>(nullptr),
0)
{
reset();
}
{ }

void
parser::
Expand Down
11 changes: 5 additions & 6 deletions include/boost/json/impl/stream_parser.ipp
Original file line number Diff line number Diff line change
Expand Up @@ -32,20 +32,19 @@ stream_parser(
size)
{
reset();
p_.handler().ignore_duplicate_keys = opt.ignore_duplicate_keys;
}

stream_parser::
stream_parser(
storage_ptr sp,
parse_options const& opt) noexcept
: p_(
opt,
: stream_parser(
std::move(sp),
nullptr,
opt,
static_cast<unsigned char*>(nullptr),
0)
{
reset();
}
{ }

void
stream_parser::
Expand Down
Loading