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

object::stable_erase #749

Merged
merged 5 commits into from Sep 18, 2022
Merged

Conversation

gummif
Copy link
Contributor

@gummif gummif commented Aug 29, 2022

Support for erasing an element without reordering. This is part one of support #748, part two would be stable_insert.

@vinniefalco
Copy link
Member

impressive work :) you figured out how object is implemented...

@codecov
Copy link

codecov bot commented Aug 29, 2022

Codecov Report

Merging #749 (47f2ad5) into develop (2cbc263) will increase coverage by 0.00%.
The diff coverage is 100.00%.

Additional details and impacted files

Impacted file tree graph

@@           Coverage Diff            @@
##           develop     #749   +/-   ##
========================================
  Coverage    99.13%   99.13%           
========================================
  Files           69       69           
  Lines         6556     6584   +28     
========================================
+ Hits          6499     6527   +28     
  Misses          57       57           
Impacted Files Coverage Δ
include/boost/json/object.hpp 100.00% <ø> (ø)
include/boost/json/impl/object.ipp 100.00% <100.00%> (ø)

Continue to review full report at Codecov.

Legend - Click here to learn more
Δ = absolute <relative> (impact), ø = not affected, ? = missing data
Powered by Codecov. Last update 2cbc263...47f2ad5. Read the comment docs.

@cppalliance-bot
Copy link

@cppalliance-bot
Copy link

@vinniefalco
Copy link
Member

@grisumbras I guess I have no objection to offering this feature. If the user wants to opt-in to preserving order at a computational cost, I can't see a downside. But its up to you, maybe there's something I haven't thought of? What does Peter think?

Copy link
Contributor

@sehe sehe left a comment

Choose a reason for hiding this comment

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

Got nerd sniped by this PR. Nice work :)

@@ -1567,6 +1614,12 @@ class object
destroy(
key_value_pair* first,
key_value_pair* last) noexcept;

inline
Copy link
Contributor

Choose a reason for hiding this comment

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

Could this use a little javadoc even though it's internal?

Copy link
Member

Choose a reason for hiding this comment

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

eh..

by the way docca supports @param first, last The range to destroy now

@par Exception Safety
No-throw guarantee.

@return An iterator following the last removed element.
Copy link
Contributor

Choose a reason for hiding this comment

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

"last" seems redundant here

Remove the element pointed to by `pos`, which must
be valid and dereferenceable. Thus the @ref end()
iterator (which is valid but cannot be dereferenced)
cannot be used as a value for `pos`.
Copy link
Contributor

Choose a reason for hiding this comment

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

The sentences "Thus the @ref end() iterator (which is valid but cannot be dereferenced) cannot be used as a value for pos." is redundant.

If this is expected to be a gotcha (?) I'd move it to a note below.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

It looks like this is verbatim from https://en.cppreference.com/w/cpp/container/vector/erase. Moved to note section.

@cppalliance-bot
Copy link

include/boost/json/impl/object.ipp Show resolved Hide resolved
include/boost/json/object.hpp Outdated Show resolved Hide resolved
include/boost/json/object.hpp Outdated Show resolved Hide resolved
@cppalliance-bot
Copy link

@grisumbras grisumbras merged commit 47f2ad5 into boostorg:develop Sep 18, 2022
@grisumbras
Copy link
Member

Thanks for the feature.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

None yet

5 participants