Skip to content

Smart pointers in TextBuffer #2

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

Draft
wants to merge 50 commits into
base: master
Choose a base branch
from
Draft

Smart pointers in TextBuffer #2

wants to merge 50 commits into from

Conversation

aminya
Copy link
Member

@aminya aminya commented Dec 31, 2020

The benchmarks are great.

There is a crash, but Nan disables exepctions. We need to add the debug build.

aminya added 30 commits December 2, 2020 20:06
constexpr variable is guaranteed to have a value available at compile 
time. whereas static const members or const variable could either mean a 
compile time value or a runtime value.
The codebase uses variables that are called `_edit`, `_ses`, etc.

We should fix these later
This replaces the deprecated c headers with their <c*> versions
 constructors must be marked explicit to avoid unintentional implicit conversions
single-argument constructors must be marked explicit to avoid 
unintentional implicit conversions 
[google-explicit-constructor,hicpp-explicit-conversions]
constructors must be marked explicit to avoid 
unintentional implicit conversions 
[google-explicit-constructor,hicpp-explicit-conversions]
Fixes implicit conversion 'int' -> bool
To prevent implicit conversion 'int/uint' -> bool
Prevents implicit conversion of the object to bool
@aminya aminya changed the title Smart pointers Smart pointers in TextBuffer Dec 31, 2020
Patch TextBuffer::get_inverted_changes(const shared_ptr<Snapshot> snapshot) const {
vector<const Patch*> patches;
auto layer = top_layer;
while (layer.get() != &snapshot->base_layer) {
Copy link
Member Author

@aminya aminya Jan 8, 2021

Choose a reason for hiding this comment

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

Not sure about this (get)

Copy link

@jeff-hykin jeff-hykin Jan 21, 2021

Choose a reason for hiding this comment

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

iIf base_layer is a layer, and not a pointer, then I believe this is correct. The left side is the address being pointed to, and the right side would be the address of the base layer.

Although &(snapshot->base_layer) would probably be more friendly to people not intimately familiar with C++ order of operations

Might want to use naming like layer_ptr = top_layer_ptr; To clarify which ones are layers, which are pointers, and which (if any) are double pointers (layer_ptr_ptr). This would make comparisons a whole lot more obvious.

Choose a reason for hiding this comment

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

Well, seeing now that base layer is a shared pointer, I think the comparison should be against snapshot->base_layer.get()

Choose a reason for hiding this comment

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

Looks like, for comparison only, the gets aren't needed. But I'd recommend keeping them to be explicit that the shared pointers themselves are not being compared.

@@ -671,18 +676,17 @@ struct TextBuffer::Layer {
};

TextBuffer::TextBuffer(u16string &&text) :
base_layer{new Layer(move(text))},
base_layer{make_shared<Layer>(Layer(move(text)))},
Copy link
Member Author

Choose a reason for hiding this comment

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

Not sure if calling Layer() explicitly is required (instead of just writing make_shared).

Choose a reason for hiding this comment

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

I think It shouldn't make a difference... unless the layer constructor needs to be called twice for some reason

Comment on lines +386 to +393
Patch &Patch::operator=(Patch &other) {
std::swap(root, other.root);
std::swap(left_ancestor_stack, other.left_ancestor_stack);
std::swap(node_stack, other.node_stack);
std::swap(change_count, other.change_count);
merges_adjacent_changes = other.merges_adjacent_changes;
return *this;
}
Copy link
Member Author

@aminya aminya Jan 8, 2021

Choose a reason for hiding this comment

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

Basically copy-pasted the move assignment operator. 😄

Choose a reason for hiding this comment

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

Shouldn't assignment be non-destructive though? (Shouldn't other be const)

Comment on lines +322 to +325
Patch::Patch(Patch &other)
: root{nullptr}, change_count{other.change_count}, merges_adjacent_changes{other.merges_adjacent_changes} {
*this = other;
}
Copy link
Member Author

Choose a reason for hiding this comment

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

😄 copy-pasted the move constructor

@@ -36,7 +36,9 @@ class Patch {
};

// Construction and destruction
Patch(Patch &);
Copy link
Member Author

Choose a reason for hiding this comment

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

not const

Patch(Patch &&);
Patch &Patch::operator=(Patch &other);
Copy link
Member Author

Choose a reason for hiding this comment

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

not const

@aminya
Copy link
Member Author

aminya commented Jan 8, 2021

@jeff-hykin It might be good to have someone else's eye on the code 😄

@jeff-hykin
Copy link

Yeah 😬 I was hoping over break I'd have time but that didn't work out and this next semester is going to be more busy than the last.

@@ -1090,11 +1083,9 @@ class SaveWorker : public Nan::AsyncWorker {

Local<Value> Finish() {
if (error) {
delete snapshot;
return error_to_js(*error, encoding_name, file_name);

Choose a reason for hiding this comment

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

Is it fine not setting the snapshot to nullptr in these cases?

Comment on lines +386 to +393
Patch &Patch::operator=(Patch &other) {
std::swap(root, other.root);
std::swap(left_ancestor_stack, other.left_ancestor_stack);
std::swap(node_stack, other.node_stack);
std::swap(change_count, other.change_count);
merges_adjacent_changes = other.merges_adjacent_changes;
return *this;
}

Choose a reason for hiding this comment

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

Shouldn't assignment be non-destructive though? (Shouldn't other be const)

Patch TextBuffer::get_inverted_changes(const shared_ptr<Snapshot> snapshot) const {
vector<const Patch*> patches;
auto layer = top_layer;
while (layer.get() != &snapshot->base_layer) {
Copy link

@jeff-hykin jeff-hykin Jan 21, 2021

Choose a reason for hiding this comment

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

iIf base_layer is a layer, and not a pointer, then I believe this is correct. The left side is the address being pointed to, and the right side would be the address of the base layer.

Although &(snapshot->base_layer) would probably be more friendly to people not intimately familiar with C++ order of operations

Might want to use naming like layer_ptr = top_layer_ptr; To clarify which ones are layers, which are pointers, and which (if any) are double pointers (layer_ptr_ptr). This would make comparisons a whole lot more obvious.

@@ -671,18 +676,17 @@ struct TextBuffer::Layer {
};

TextBuffer::TextBuffer(u16string &&text) :
base_layer{new Layer(move(text))},
base_layer{make_shared<Layer>(Layer(move(text)))},

Choose a reason for hiding this comment

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

I think It shouldn't make a difference... unless the layer constructor needs to be called twice for some reason

Patch TextBuffer::get_inverted_changes(const shared_ptr<Snapshot> snapshot) const {
vector<const Patch*> patches;
auto layer = top_layer;
while (layer.get() != &snapshot->base_layer) {

Choose a reason for hiding this comment

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

Well, seeing now that base layer is a shared pointer, I think the comparison should be against snapshot->base_layer.get()

Patch TextBuffer::get_inverted_changes(const shared_ptr<Snapshot> snapshot) const {
vector<const Patch*> patches;
auto layer = top_layer;
while (layer.get() != &snapshot->base_layer) {

Choose a reason for hiding this comment

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

Looks like, for comparison only, the gets aren't needed. But I'd recommend keeping them to be explicit that the shared pointers themselves are not being compared.

@@ -784,7 +787,7 @@ void TextBuffer::serialize_changes(Serializer &serializer) {

bool TextBuffer::deserialize_changes(Deserializer &deserializer) {
if (top_layer != base_layer || (base_layer->previous_layer != nullptr)) return false;

Choose a reason for hiding this comment

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

Looks like C++2020 is going to be removing the != operator for shared pointers.
https://en.cppreference.com/w/cpp/memory/shared_ptr/operator_cmp
So might want to consider converting all the comparisons for compatibility with 2020

@aminya
Copy link
Member Author

aminya commented Jan 22, 2021

@jeff-hykin Thanks for the comments! I will go through them

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.

2 participants