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

Explicitly use std::deque for the missing node handler stack: #2252

Closed
wants to merge 1 commit into from
Closed

Explicitly use std::deque for the missing node handler stack: #2252

wants to merge 1 commit into from

Conversation

nbougalis
Copy link
Contributor

We need to ensure that pointers and/or references to existing elements will not be invalidated during the course of element insertion and removal.

Containers like std::vector do not offer this guarantee, and cannot be used as the underlying container for the stack.

By choosing to explicitly specify std::deque as the underlying cotnainer, we avoid:

  • the unlikely possibility of the C++ standards committee changing the default template parameter to a container;
  • the more likely possibility of an accidental change by a programmer, without fully considering the consequences.

We need to ensure that pointers and/or references to existing
elements will not be invalidated during the course of element
insertion and removal.

Containers like std::vector do not offer this guarantee, and
cannot be used as the underlying container for the stack.

By choosing to explicitly specify std::deque as the underlying
cotnainer, we avoid:

- the unlikely possibility of the C++ standards committee
  changing the default template parameter to a container;
- the more likely possibility of an accidental change by
  a programmer, without fully considering the consequences.
@codecov-io
Copy link

Codecov Report

Merging #2252 into develop will decrease coverage by 0.01%.
The diff coverage is n/a.

Impacted file tree graph

@@            Coverage Diff             @@
##           develop   #2252      +/-   ##
==========================================
- Coverage    70.11%   70.1%   -0.02%     
==========================================
  Files          689     689              
  Lines        50800   50800              
==========================================
- Hits         35617   35611       -6     
- Misses       15183   15189       +6
Impacted Files Coverage Δ
src/ripple/shamap/SHAMap.h 100% <ø> (ø) ⬆️
src/ripple/basics/DecayingSample.h 77.77% <0%> (-8.34%) ⬇️
src/ripple/app/misc/SHAMapStoreImp.cpp 78.92% <0%> (-1.03%) ⬇️
src/ripple/core/impl/JobQueue.cpp 86.85% <0%> (-0.47%) ⬇️
src/ripple/protocol/impl/Serializer.cpp 70.3% <0%> (+0.34%) ⬆️
src/ripple/core/Stoppable.h 100% <0%> (+6.66%) ⬆️

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 cafe18c...6785515. Read the comment docs.

@HowardHinnant HowardHinnant self-requested a review October 26, 2017 02:05
Copy link
Contributor

@HowardHinnant HowardHinnant left a comment

Choose a reason for hiding this comment

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

Practically perfect in every way! :-)

@bachase bachase added the Passed Passed code review & PR owner thinks it's ready to merge. Perf sign-off may still be required. label Nov 29, 2017
@bachase
Copy link
Collaborator

bachase commented Nov 30, 2017

Merged as f0e1024

@bachase bachase closed this Nov 30, 2017
@nbougalis nbougalis deleted the deque branch March 22, 2018 03:29
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
Passed Passed code review & PR owner thinks it's ready to merge. Perf sign-off may still be required.
Projects
None yet
Development

Successfully merging this pull request may close these issues.

None yet

4 participants