Add this suggestion to a batch that can be applied as a single commit.
This suggestion is invalid because no changes were made to the code.
Suggestions cannot be applied while the pull request is closed.
Suggestions cannot be applied while viewing a subset of changes.
Only one suggestion per line can be applied in a batch.
Add this suggestion to a batch that can be applied as a single commit.
Applying suggestions on deleted lines is not supported.
You must change the existing code in this line in order to create a valid suggestion.
Outdated suggestions cannot be applied.
This suggestion has been applied or marked resolved.
Suggestions cannot be applied from pending reviews.
Suggestions cannot be applied on multi-line comments.
Suggestions cannot be applied while the pull request is queued to merge.
Suggestion cannot be applied right now. Please check back later.
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
feat: Implement basic in-memory store and change handling. #165
feat: Implement basic in-memory store and change handling. #165
Changes from 21 commits
95bab24
a4b2c8c
d368988
48bfc55
758e825
6914703
c74f508
cd1ec6f
e82dc04
54499d6
1df05f9
5221784
e9734bc
b53b004
1d07b00
58d284f
f6b9558
d4a2a9d
091bcb8
f3e0a5d
0bd9846
2fcca51
eedb155
f2f4fe6
2fb9abb
b33ae7f
e2d205a
0561288
e530857
e62258b
057355e
a3614f7
e3fadc0
b8fb098
fe27583
7eb0023
5cef5f3
6618806
662dbd9
b812099
File filter
Filter by extension
Conversations
Jump to
There are no files selected for viewing
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Ambiguous reading if this is a function or constructor. Maybe just
?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Is the comment meant to literally mean "moved" into the store i.e.
std::move
? (because that would seem to be a good use of move here, if the compiler can't figure it out.)There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
There should be a move here as well, but the comment refers to the actual descriptors.
std::make_shared<IDataStore::FlagDescriptor>(std::move(flag));
In that we move the flag out of this descriptor and into the descriptor in the actual store.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I think I might've made some code suggestions that were wrong (
std::string
tostd::string const&
), since I'm realizing you might've had a strategy.for all these functions that take
key
by value, they could be const&. But maybe deep down the key is being moved into a map or whatever?There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
That was the plan, but I think it got lost in the layers.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I may just make more things const and let it copy, otherwise it is really easy to use the key twice in one function and it to be invalid. Downside of the way C++ does moves.