Add a nanopb string #1839
Add a nanopb string #1839
Conversation
There was a problem hiding this comment.
(github ate my comments. I've added them back. Hopefully you only end up with one set. :/ )
As discussed, the general approach seems ok, and I can't immediately think of any reason as to why this would be a bad idea.
Although you didn't intend it as such, I've reviewed this as an actual PR. So there's probably some redundant comments.
| class String : public util::Comparable<String> { | ||
| public: | ||
| static pb_bytes_array_t* MakeBytesArray(absl::string_view value) { | ||
| auto size = static_cast<pb_size_t>(value.size()); |
There was a problem hiding this comment.
This won't work if string_view contains wchar_t's rather than char's. Fortunately, absl::string_view only supports chars, so we should be ok.
But if you want to future proof us for std::string_view (which does support wchar_t's) then you could either:
a) Add an assert that checks the size/type of string_view::value_type.
b) Actually perform the math. (Dubious. The extra complication seems unwarranted just now since it can't possibly be useful until we absl::string_view -> std::string_view.)
There was a problem hiding this comment.
Even once we move from absl::string_view to std::string_view (in several years once we can require C++17 support as a floor) it's still the case that the wide version would be std::wstring_view. This code doesn't attempt to handle std::basic_string_view and I don't think it ever will, so I think we can safely assume sizeof(decltype(value)::value_type) == 1.
|
|
||
| class String : public util::Comparable<String> { | ||
| public: | ||
| static pb_bytes_array_t* MakeBytesArray(absl::string_view value) { |
There was a problem hiding this comment.
Ideally yes, but if there's any need to create the pb_bytes_array_t directly we should funnel them all through one implementation. For example, you have a parallel implementation in Serializer::EncodeString that doesn't null terminate the result and I think that's less useful than it could be.
| namespace firestore { | ||
| namespace nanopb { | ||
|
|
||
| class String : public util::Comparable<String> { |
There was a problem hiding this comment.
docstrings might be nice. Specifically, something mentioning that the ctor copies it's arguments.
There was a problem hiding this comment.
Yup. As submitted I was trying to figure out if it was worth investing in such things.
|
|
||
| String& operator=(String other) { | ||
| using std::swap; | ||
| swap(*this, other); |
There was a problem hiding this comment.
Spelling swap as std::swap would allow you to eliminate the using statement. Also below.
Questions: