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

String optimization #6

Open
chungphb opened this issue Jul 3, 2021 · 7 comments
Open

String optimization #6

chungphb opened this issue Jul 3, 2021 · 7 comments
Labels
string Related to string

Comments

@chungphb
Copy link
Owner

chungphb commented Jul 3, 2021

Various ways to optimize spiderdb::string

@chungphb
Copy link
Owner Author

chungphb commented Jul 3, 2021

Since sizeof(char) is always 1, should we use std::malloc(len) instead of std::malloc(sizeof(char_t) * len)?

Should we use static_cast<char*> or change it to reinterpret_cast<char*>?

@chungphb
Copy link
Owner Author

chungphb commented Jul 3, 2021

Should we just remove the basic_string(char_t* data, size_t len) and basic_string(seastar::temporary_buffer<char_t>&& buffer) constructors and allow constructing by basic_string(const char_t* data, size_t len) only?

@chungphb
Copy link
Owner Author

chungphb commented Jul 3, 2021

For copy assignment operator and move assignment operator, we should call destructor to destroy the old object before constructing a new one.

For move constructor and move assignment operator, the moved object's destructor should also be called. After that, we won't need to reset its members anymore.

When we move a String object, we won't allocate a new block but reuse the old one and transfer its ownership to the new object. If we called the moved object's destructor, this block will be deallocated and the new object's _data member will be a dangling pointer.

@chungphb
Copy link
Owner Author

chungphb commented Jul 3, 2021

String errors should be preceded by "string: " or "spiderdb::string:" to remove capitalism altogether (no pun intended).

Also, why do we want to allow constructing a string with length 0?

@chungphb
Copy link
Owner Author

chungphb commented Jul 3, 2021

We should replace basic_string(std::basic_string<char_t> str) by basic_string(const std::basic_string&<char_t> str) and basic_string(const std::basic_string<char_t>& str). There's no need to create a new string just to deleted it right after.

@chungphb
Copy link
Owner Author

chungphb commented Jul 3, 2021

For operator[](size_t id)-s, there's no need to compare id to 0 since it's a size_t parameter.

For operator+(const basic_string& str), instead of constructing a new string using the basic_string(size_t len, char_t c) constructor so we could set all the characters to 0 for no reason before reset it immediately, we should just simply allocate the memory in place.

For operator==(const basic_string<char_t>& str), we could use std::equal instead of writing our own for loop for readability.

For operator>(const basic_string<char_t>& str) and operator<(const basic_string<char_t>& str), we could write a compare function (similar to seastar::sstring) to reduce redundancy.

All the comparison operators (==, !=, >, <. >=, <=) should be noexcept.

@chungphb
Copy link
Owner Author

chungphb commented Jul 3, 2021

We should move get_input_stream() out of String class.

Also, there's no need to make std::hash<basic_string> and operator<<(std::ostream& os, basic_string str) friends of String class when we already provided enough functions for them to use.

We also might want to be able to create an std::basic_string_view object from a String object to take advantages of its lightweight nature.

@chungphb chungphb added the string Related to string label Jul 6, 2021
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
string Related to string
Projects
None yet
Development

No branches or pull requests

1 participant