-
Notifications
You must be signed in to change notification settings - Fork 1.2k
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
fix toString function defect #345
Conversation
Can one of the admins verify this patch? |
src/graph/mock/PropertiesSchema.cpp
Outdated
buf += "] "; | ||
buf += columnTypeToString(item.type_); | ||
buf += ","; | ||
std::string propItemStr = item.name_; |
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.
Before you do append string, reserve enough space will be more efficient.
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.
My bad. I just tested it on my machine, by default, the capacity of empty string is fifteen.
👍
Fix comments, please review again, thx. |
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.
You are making things neither simpler nor more performant.
If you already have an collection of strings to be joined, using folly:join
is reasonable since it's more concise. Otherwise, if you collect them manually first, you introduce additional copy movements, which is way too inefficient.
My opinion is to review all of the resize
s to guarantee they are doing correctly.
fix and rebase master, please review, thx. |
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.
Thanks for this enhancing job!
Jenkins, go! |
Unit testing passed. |
…-inc#345) * fix toString function defect * address dangleptr's comments * address dutor's comments
…-inc#345) * fix toString function defect * address dangleptr's comments * address dutor's comments
* feat - support chinese * fix - remove debug anno * support `chinese` * add test cases * fix encode bug Co-authored-by: cpw <[email protected]> Co-authored-by: Yee <[email protected]> Co-authored-by: Sophie <[email protected]> Co-authored-by: endy.li <[email protected]> Co-authored-by: cpw <[email protected]> Co-authored-by: Yee <[email protected]> Co-authored-by: Sophie <[email protected]>
fix toString function defect