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

cstring spring cleaning #368

Merged
merged 4 commits into from
Mar 21, 2017

Conversation

sethfowler
Copy link
Contributor

I was writing some cstring-heavy code recently and this prompted me to take a look at cstring.h. I was glad I did, because it turned out I hadn't quite understood how cstring was intended to behave! Though I know there is documentation of cstring elsewhere, I felt like this was a sign that it'd be good to have some documentation in the code. 70c2aa3 contains everything I've learned about cstring so far; if I've misunderstood or missed anything, let me know! fa5adaf builds on that with a little bit of cleanup.

In the course of putting cstring under the lens I noticed a couple of minor issues that I've also fixed in this PR.

c21f56f adds a cstring constructor which takes a pair of iterators; this is necessary for many STL and Boost algorithms to work.

3fa944e removes a performance gotcha when using cstring: constructing a cstring from an std::string (either via the std::string constructor/assignment operator, or indirectly via the std::string stream or pair-of-iterators constructors) involves calling c_str() on the std::string to get a const char*, and then calling cstring::operator=(const char*), which creates a new std::string from the const char* and checks if it's present in the cache. Even if it's in the cache already, though, that std::string copy will still have been created. 3fa944e avoids this by adding an additional cstring::operator=(const std::string&) overload which checks the cache using the existing std::string instance. This should result in significantly fewer unnecessary copies when converting between std::string and cstring.

@sethfowler sethfowler merged commit 67a4170 into p4lang:master Mar 21, 2017
@sethfowler sethfowler deleted the seth/cstring-spring-cleaning branch March 21, 2017 20:00
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