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
mds: convert const char ptr to string_view #25921
Conversation
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
954ee6a
to
f1679cf
Compare
Also, don't store pointers to strings outside. The interface was written to optimize for string literals but this is structurally unsafe programming. Signed-off-by: Patrick Donnelly <pdonnell@redhat.com>
45ae2c5
to
d284ca0
Compare
Trivial refactor. Signed-off-by: Patrick Donnelly <pdonnell@redhat.com>
d284ca0
to
6dfa87f
Compare
* refs/pull/25921/head: mds: convert const char ptr to string_view common/TrackedOp: use string_view interface Reviewed-by: Sage Weil <sage@redhat.com>
@@ -234,35 +234,26 @@ class TrackedOp : public boost::intrusive::list_base_hook<> { | |||
|
|||
struct Event { | |||
utime_t stamp; | |||
string str; | |||
const char *cstr = nullptr; | |||
std::string str; |
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.
this forces us to create a std::string
from string_view
. IMHO, it practically defeats the purpose of using string_view
. i'd use std::variant<>
for holding the string_view
or std::string
.
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.
The std::string_view
may not refer to static memory? In several places, an Event is created with s
referring to a temporary string.
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 think we can differentiate that case at the caller site, can't we? and instead move the temporary string or copy it to the one in Event
.
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 agree with you. See my comment here: #28849 (comment)
No description provided.