-
Notifications
You must be signed in to change notification settings - Fork 440
feat(bigtable): add Clear(std::string&) helper for RowKeyType compatibility #15252
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
base: main
Are you sure you want to change the base?
feat(bigtable): add Clear(std::string&) helper for RowKeyType compatibility #15252
Conversation
610e85c to
dee1b25
Compare
|
/gcbrun |
Codecov Report✅ All modified and coverable lines are covered by tests. Additional details and impacted files@@ Coverage Diff @@
## main #15252 +/- ##
=======================================
Coverage 92.96% 92.96%
=======================================
Files 2402 2402
Lines 217556 217556
=======================================
+ Hits 202244 202251 +7
+ Misses 15312 15305 -7 ☔ View full report in Codecov by Sentry. 🚀 New features to boost your workflow:
|
| inline bool IsEmptyRowKey(std::string const& key) { return key.empty(); } | ||
|
|
||
| /// Clear the row key. | ||
| inline void Clear(std::string& key) { key.clear(); } |
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 would suggest not placing this in between two of the implementations of IsEmptyRowKey().
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 thought that was the right place logically because of the context around IsEmptyRowKey(), but I might be missing a broader perspective. Could you please suggest a more appropriate place for this? Thanks!
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 would suggest placing Clear() before all the IsEmptyRowKey()s, or after all of them ... not in the middle.
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.
done
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.
It looks like you moved Clear() from between the first and second implementations of IsEmptyRowKey() to between the second and the third.
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.
Really Sorry, Now I Correct It
|
/gcbrun |
scotthart
left a comment
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.
Reviewed 1 of 1 files at r3, all commit messages.
Reviewable status: all files reviewed, 1 unresolved discussion (waiting on @devbww)
|
/gcbrun |
…bility Adds a Clear(std::string&) function that calls std::string::clear() in OSS builds. This unifies the interface for clearing row keys between OSS (std::string) and Google internal (absl::Cord), where an overload will call absl::Cord::Clear() instead of deprecated clear(). Signed-off-by: Dronaraj Gyawali <dronarajgyawali@gmail.com>
Signed-off-by: Dorna Raj Gyawali <dronarajgyawali@gmail.com>
d027ba7 to
78b644c
Compare
|
/gcbrun |
This pr solve the issue #15248
Adds Clear(std::string& key) function that calls std::string::clear() in OSS builds. Google-land will have corresponding overload for absl::Cord::Clear() to replace deprecated absl::Cord::clear().
This change is