Change Redis query type from String to an optional std::string_view#10391
Change Redis query type from String to an optional std::string_view#10391
String to an optional std::string_view#10391Conversation
|
Strange, it worked on my machine... |
|
The idea looks fine in general. Besides getting it to work, you might want to take a look at |
|
Like, |
|
Probably something like that. A function for it might work too, but a small class could be nicer overall. |
ab1d887 to
7a5d6f3
Compare
050665e to
cfc5532
Compare
cfc5532 to
12e8c73
Compare
|
Fine, @yhabteab, you have won... |
12e8c73 to
d8a0d08
Compare
yhabteab
left a comment
There was a problem hiding this comment.
Can you please use an appropriate line wrapping for your commit description in d8a0d08 (like 80-120), so that git log can render it properly, isntead of soft wrap it at random location because my screen is too small for it. Please also fix the new compiler warnings introduced with this PR.
String to an optional std::string_view
|
Yeah, stupid git-log(1) cuts off at a random character in the middle of the word. Only hard wrapping helps here. @lippserd said 72 for title, 80 for description. |
d8a0d08 to
3d4e853
Compare
It was copied anyway.
to ease changing that type in the future.
This expresses what kind of vector it is and allows to easily change those types in the future.
3d4e853 to
e9213ef
Compare
e9213ef to
66993aa
Compare
yhabteab
left a comment
There was a problem hiding this comment.
LFTM now, but please do not merge it yet!
jschmidt-icinga
left a comment
There was a problem hiding this comment.
The change request is for the broken move constructor. But I want to at least hear a good explanation about why this has to derive from std::string_view.
1bb2e0f to
7fc6db6
Compare
Especially our history messages contain lots of hardcoded C string literals `"like this one"`. At runtime, they get translated to pointers to constant global memory, `const char*`. String `malloc(3)`s and copies these data every time. In contrast, the new type just stores the address if any. (Actually, `const char*` is wrapped by `std::string_view` to not compute its length every time.)
7fc6db6 to
1702eeb
Compare
jschmidt-icinga
left a comment
There was a problem hiding this comment.
QueryArg looks good to me now 👍. I'm happy that it actually ended up not adding complexity, maybe even made the PR a bit simpler.
I haven't looked in detail at where it's used and why, but I trust @yhabteab's review on that.
(Find TL;DR below)
Seriously speaking, 70ca88a pollutes the diff and should be viewed on its own. It just replaces
std::vector<String>with RedisConnection::Query. As RedisConnection::Query already was astd::vector<String>, this is a no-op on its own. But it allows to just change RedisConnection::Query itself and affect all its usages at once.Why do we have to change String to std::variant<const char*,String> at all? (efe5f22)
Especially our history messages contain lots of hardcoded C string literals "like this one". At runtime, they get translated to pointers to constant global memory, const char*. String malloc(3)s and copies these data every time. In contrast, std::variant<const char*,String> just stores the address if any.
TL;DR
std::vector<T>fixes #10276
ref/NC/820479