-
Notifications
You must be signed in to change notification settings - Fork 5.9k
Add StringPiece #2432
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
Add StringPiece #2432
Conversation
paddle/strings/stringpiece.h
Outdated
|
|
||
| // StringPiece points into a std::string object but doesn't own the | ||
| // string. It is for efficient access to strings. Like Go's string | ||
| // type. StringPiece is not thread-safe. |
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.
Since StringPiece is immutable, it would be thread-safe as long as the underlaying string is not mutated during the lifetime of StringPiece.
Maybe we could change "StringPiece is not thread-safe." to "StringPiece is thread-safe as long as the underlying string is not mutated."?
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.
Doing this!
| } | ||
|
|
||
| StringPiece::StringPiece(const char* s) : data_(s) { | ||
| size_ = (s == NULL) ? 0 : strlen(s); |
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.
if we use strlen here, we are limiting ourselves to c-string that ends with 0.
Perhaps StringPiece::StringPiece(const char* s, int size) could be more general (supports any char array, could have 0 in the body). Since std::string supports 0 in the body, maybe we need to support as well.
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.
Got it. If we have a C string const char* a, we can create a StringPiece using
StringPiece s(std::string(a));Done, delete it.
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.
Oh. I tried it and it seems that we should leave it there; otherwise, we would have to write the following test assertion
EXPECT_EQ("app", TrimSuffix(StringPiece("apple"), "pl"));in a much more complicated way:
EXPECT_EQ(std::string("app"),
TrimSuffix(StringPiece(std::string("apple")), StringPiece(std::string("pl")));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 see, I think we can keep it. LGTM!
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.
LGTM after StringPiece::StringPiece(const char* s) is deleted.
* Add S2ANet model for Oriented Object Detection.
Fixes #2428
I borrowed ideas from the following StringPiece implementations:
stringspackage