[De]Serialize FieldValue map_values ("Objects")

[rsgowman]

These can (recursively) contain other FieldValues.

[zxu123]

LGTM

[zxu123]

I have difficulty to understand what this function is for without looking into implementation. Could you either add docs or change naming?

[wilhuff]

Both please.

[rsgowman]

The name follows the same pattern as the rest: EncodeType, so I'd like to keep that. I've added some docs PTAL and let me know if that clears it up.

Also, there's a minor spelling mistake in the name: FieldEntry -> FieldsEntry.

[var-const]

For naming: I find it a little non-intuitive that an Object created from a map consists of FieldEntries (I understand this is Nanopb terminology). Perhaps MapEntry or ObjectMember?

[rsgowman]

Unfortunately, this method is (currently) specific to FieldsEntry's. It won't work for generic entries in a map. (Note the references to google_firestore_v1beta1_MapValue_FieldsEntry_key_tag and google_firestore_v1beta1_MapValue_FieldsEntry_value_tag within the implementation.)

Would EncodeMapValueFieldsEntry() be clearer?

[var-const]

Partial review.

[var-const]

1. Was the intention to return a const reference? Otherwise, just skip const and return a non-const value. It doesn't prevent the caller from storing this return value in a non-const variable, and it prevents move semantics.
2. Map's value_type is already defined as having const keys (std::pair<const K, V>), so I suggest you drop const from std::string template parameter (here and in other declarations).
3. I see that this declaration is reused in serializer. Perhaps introduce an alias? using ObjectT = std::map<...>;

[rsgowman]

1. Was the intention to return a const reference? ...

Whoops; yeah was supposed to be a reference. (Fixed.)

2 ...drop const from std::string template parameter...

I agree, but that's a separate change which I'll leave for another CL. ... Hm. No, I changed my mind. That's trivial enough that I'll just do it now. (Fixed.)

3 ...Perhaps introduce an alias? using ObjectT = std::map<...>;

I have no strong opinion on this. Generally, I prefer to avoid convenience aliases as I like to see what the actual type is. But I admit that this would be useful to help trim down the boilerplate. Other votes?

[var-const]

Nit: I don't think these names can conflict as far as overloading resolution is concerned. I presume your aim is to distinguish them clearer for the reader? If that's the case, consider rephrasing to something like Named '...Impl' to clearly distinguish from Serializer::EncodeFieldValue (strawman).

[rsgowman]

Unfortunately, they do conflict. I renamed this to allow the compiler to distinguish them rather than the reader. I originally assumed that they would not, but the compiler doesn't seem to look further once it sees the method present in the class. :(

Update: We decided to keep this for now, but to refactor into a wrapper class, so this will shortly disappear.

[var-const]

Ah, sorry, I missed your comment with the explanation. So one shadows the other. It's also possible to use the free function as ::EncodeFieldValue, but I guess this code will change quite a bit after refactoring anyway.

[rsgowman]

That doesn't work either. (It was actually the first thing I though to try.) I presume because it's not actually in the global namespace. And yeah, it should change significantly after refactoring.

[var-const]

Yes, the declaration in the class shadows the declaration in the containing namespace, so the free function doesn't even participate in the overload resolution. I'm pretty sure that ::firebase::firestore::whatever::NameOfFunction should work (sorry I communicated it poorly), but if it doesn't, guess I'm wrong.

[var-const]

Shouldn't this variable name be snake_case?

[rsgowman]

Yup; fixed.

[var-const]

1. What's the meaning of 0x42?
2. Entirely optional: consider std::array. It will be somewhat more verbose, but slightly higher-level:

std::array<uint8_t, 1024> buf;
buf.fill(0x42);
pb_ostream_t stream = pb_ostream_from_buffer(buf.data(), buf.size());

[rsgowman]

Oops. This was debugging stuff that I forgot to remove. I don't actually care what it's initialized to. (Removed.)

[var-const]

1. This comment should explain the purpose of this check. Also, why not use 922... number directly? In case you deliberately test for max() here, it would also be great to explain that in the comment.
2. This can probably be a static_assert. As far as I can tell, VS 2015 should have numeric_limits<T>::max() as constexpr.

[rsgowman]

I didn't want to duplicate the comment from above. Regardless, this (and all other raw mins/maxes) should disappear as soon as we link libprotobuf to the test suite. (Added TODO, but otherwise haven't taken an action as this code will be deleted soon.)

[var-const]

I understand it might be preferable to be explicit here, but in general, does our FieldValue interface require calling the static "constructors"? I.e., is it not possible to have:

{"i", 1}, {"n", FieldValue::NullValue()}, {"s", "foo"}, ...

[rsgowman]

Currently yes. But I'd certainly not object to allowing FieldValues to be constructed like that. (I'll bring it up for discussion in the chat.)

[wilhuff]

std::map already treats the key as const and FieldValue is already immutable. I love const as much as the next guy but this seems like a pretty bonkers type.

[rsgowman]

Just after I had removed it from the key. :)

Now removed from value too.

[wilhuff]

If forward declarations really are required, please put them up at the top of the file so we don't have to hunt for them.

Also, laid out together like this it took me a moment to realize this wasn't a single declaration. Maybe newline in between?

[rsgowman]

both done.

[wilhuff]

This isn't a useful comment. It focuses on why the names are different without drawing a distinction between the differences in behavior which is the actually interesting part.

Consider two audiences:

• The first-time reader wading in here for the first time: help them tame the complexity in this file. Help understand the differences between these two. Why would I choose one or the other?
• The over-zealous new project member that wants to refactor everything: why are these separate? Do they need to be? What breaks if you use the wrong one?

[rsgowman]

Yeah, see my previous (long buried) comment on this PR. Or just ignore it. We decided we can probably convert this into a class that wraps the stream struct which will likely cause a number of good things to happen (incl the elimination of this.)

Added a TODO for now.

[wilhuff]

Since you're already using a sizing stream let's just do everything via the sizing stream. The less we have to know the specifics about the bytes layout in here the easier this code will be to work with over the long haul.

[rsgowman]

I don't think I understand this comment.

Or are you suggesting moving the sizing calculation out of this method, and up to the calling location? That would make sense, and is similar to the split between EncodeFieldValue and EncodeNestedFieldValue (although in this case, the FieldsEntry object is only every a nested message.) If so; done. I suspect that'll need to get reworked a bit more during the refactoring.

[wilhuff]

Both please.

[wilhuff]

I realize none of this is duplicate code but you've got a lot of things in here with very similar names. On the encoding side:

• EncodeFieldValueImpl
• EncodeSubFieldValue
• EncodeFieldEntry
• EncodeFieldValue

Perhaps EncodeFieldEntry is actually about maps? We need better names in here to keep these straight.

Impl vs not Impl rarely helps the reader understand what's going on. Maybe EncodeFieldValueToStream?

Alternatively, should we reserve Encode for creating nanopb struct things and come up with some other verb for writing into streams? This would also be somewhat easier to comprehend if these weren't all free functions in the same file.

Given what I can see here so far, my suggestion would be to put these things that operate on a pb_ostream into a class we can stack allocate where we need it.

[rsgowman]

This comment is mostly obsolete, except for this part:

Given what I can see here so far, my suggestion would be to put these things that operate on a pb_ostream into a class we can stack allocate where we need it.

which is exactly what we decided to do (as a followup.)

For now, I've:
a) Added docs to EncodeFieldsEntry.
b) Added docs to EncodeSubFieldValue and renamed to EncodeNestedFieldValue. (When refactoring, I suspect this might actually be eliminated and translated to a bool parameter since we'll need to do the same thing for all messages that could be top level messages.)

[var-const]

To echo today's discussion, I think there's now probably enough code to start refactoring. In particular, I'd suggest adding some nicer wrappers over Nanopb structs and functions. As it stands, the C functions are used directly, resulting in a lot of low-level details spread across the functions, so I think some intermediary interface could be helpful.

[var-const]

Please clarify what is doing the serialization here (libprotobuf, I understand).

[rsgowman]

Probably all of them. Certainly protoc. Updated docs.

[var-const]

This is probably a bad idea, but just to throw it out: I guess it's possible to have a bytes array for each field and assert that each one of those arrays is found within the resulting array, and that if each of those matches is removed, the array is empty:

// Pseudocode
uint8_t encoded_fields[][] = { {0xFF, 0xFF}, {0xAA, 0xAA, 0xAA}, ... };
auto result = TheEncodingFunction();
for (const auto& encoded_field : encoded_fields) {
  auto found = std::search(result.begin(), result.end(), encoded_field.begin(), encoded_field.end());
  assert(found != result.end());
  result.erase(found, found + encoded_field.size());
}
assert(result.empty());

[rsgowman]

I think that might even work, though would need to be recursive to handle maps within maps. But lets try the libprotobuf solution first. :)

[var-const]

1. For casting void pointers, it's preferable to use static_cast instead of reinterpret_cast.
2. Consider dereferencing the pointer to map here (there is no check for nullptr anyway) and assigning to a reference.
3. If you also introduce an alias, it could become:

ObjectT& object_value = *static_cast<const ObjectT*>(*arg);

4. I presume that this is dictated by Nanopb interface, but just to check, is it possible to just use a pointer to void instead of a pointer to pointer to void?

[rsgowman]

1. Done.
2. Done.
3. Deferred to chat.
4. No, nanopb defines it as a void**. (Curiously, it used to define it as a void*, but migrated to void** to allow it to be used as an output parameter.)

[var-const]

Nit: add a hyphen (key-value).

[rsgowman]

done.

[var-const]

What does the MapValue_fields constant do? Consider adding a comment.

[rsgowman]

It's nanopb's representation of the MapValue message fields. It's a normal nanopb thing that nanopb generates for every proto message. Up to this point, I've used the *_fields relatively infrequently, because I've had to serialize most things manually. But normal use of nanopb uses these extensively in exactly this manner. (As this class grows to handle messages that don't involve oneofs, these should appear more frequently.)

Best comment I can come up with is 'represents the MapValue message fields', but I think that's pretty low value. Other suggestions?

[var-const]

I guess what I'm missing is how does Nanopb figure out where one field ends and the other starts? Does each field contain a header? Or is there an exhaustive header for the whole map? Is this constant some sort of a flag, or a header, or something else?

[rsgowman]

It's purposely opaque; you're just supposed to pass it in to the pb_encode* methods when serializing. But if you dig into the details a bit, it's actually an (auto-generated) array of pb_field_t's, which nanopb uses to determine the structure of all the fields in the message. User code never touches this struct.

nanopb generates one of these for each message present in the .proto files.

[var-const]

I don't think emplace has any advantage over insert here, and it looks like this should use std::move.

[rsgowman]

I don't think it has any disadvantage though either, right?

"If you want insert-or-update, use operator[]. If you want insert-if-new, use emplace(). For modern C++ code, you can forget about insert()." - http://go/totw/67

re: move: done.

[var-const]

You can pretty much ignore what I wrote, since TotW of course takes priority.

Anyway, for the record, I disagree with the rationale from the URL. emplace makes it clear that the element is constructed in-place, whereas insert makes it clear that the element was fully constructed elsewhere and is being copied/moved. map is indeed a special case, but a) as is mentioned in a footnote, my_map.insert({k, v,}) isn't really worse; b) I don't find making map, where value_type is always pair, into a special case to be very helpful. E.g., in a vector:

// I presume this vector stores some numeric type. The information provided to me
// in just this line says that this vector's value_type is int or something implicitly
// convertible to int.
vec.push_back(42);
// I presume this vector does NOT store integers but something more complicated,
// e.g. it calls PackagedTask::PackagedTask(int priority). The only piece of information
// I get from this line is that this vector's value_type has a constructor taking an int.
vec.emplace_back(42);

Another angle to look at it is that insert respects, while emplace ignores explicit-ness of constructors:

struct Foo { explicit Foo(int) {} };
vector<Foo> x;
x.push_back(1); // Compilation error
x.emplace_back(1); // Ok

I understand the desire to eliminate one feature in favor of another, but for it to be worth it, IMO, it should apply uniformly, and hopefully I've argued that it's not the case for other containers, e.g. vector. Remembering that map is a special case with regards to emplace seems to me more trouble than it's worth.

[var-const]

Ironically, TotW 112 makes pretty much the same argument about vectors. So I guess I just disagree with the author on whether special-casing std::map is worth the trouble.

[var-const]

Same comments as in the Encode counterpart.

[rsgowman]

Done.

[var-const]

Same note about a potential comment on MapValue_fields.

[rsgowman]

ack

[var-const]

Nit: I think you can just return {key, value};.

[rsgowman]

done.

[var-const]

Nit: s/pb_clost/pb_close.

[rsgowman]

Done.

[var-const]

Nit: s/it's/its

[rsgowman]

Done.

[var-const]

Never mind, then. I guess I'm making some wrong assumption about Nanopb code. I presumed it's a plain C struct, in which case it's not an error to leave out some initializers when doing aggregate initialization (the rest of the members will be value-initialized). (If the number of initializer clauses is less than the number of members...).

[var-const]

You can pretty much ignore what I wrote, since TotW of course takes priority.

Anyway, for the record, I disagree with the rationale from the URL. emplace makes it clear that the element is constructed in-place, whereas insert makes it clear that the element was fully constructed elsewhere and is being copied/moved. map is indeed a special case, but a) as is mentioned in a footnote, my_map.insert({k, v,}) isn't really worse; b) I don't find making map, where value_type is always pair, into a special case to be very helpful. E.g., in a vector:

// I presume this vector stores some numeric type. The information provided to me
// in just this line says that this vector's value_type is int or something implicitly
// convertible to int.
vec.push_back(42);
// I presume this vector does NOT store integers but something more complicated,
// e.g. it calls PackagedTask::PackagedTask(int priority). The only piece of information
// I get from this line is that this vector's value_type has a constructor taking an int.
vec.emplace_back(42);

Another angle to look at it is that insert respects, while emplace ignores explicit-ness of constructors:

struct Foo { explicit Foo(int) {} };
vector<Foo> x;
x.push_back(1); // Compilation error
x.emplace_back(1); // Ok

I understand the desire to eliminate one feature in favor of another, but for it to be worth it, IMO, it should apply uniformly, and hopefully I've argued that it's not the case for other containers, e.g. vector. Remembering that map is a special case with regards to emplace seems to me more trouble than it's worth.

[var-const]

Yes, the declaration in the class shadows the declaration in the containing namespace, so the free function doesn't even participate in the overload resolution. I'm pretty sure that ::firebase::firestore::whatever::NameOfFunction should work (sorry I communicated it poorly), but if it doesn't, guess I'm wrong.

[var-const]

I guess what I'm missing is how does Nanopb figure out where one field ends and the other starts? Does each field contain a header? Or is there an exhaustive header for the whole map? Is this constant some sort of a flag, or a header, or something else?

[wilhuff]

This comment confused me initially, but I think I've come back.

Initially I thought this comment was trying to tell me something about how map_value was a recursive type and so this was required on any map_value nested within a map_value. However, you don't have any provision in here for doing anything different for the outer instance so I realized this must not be it.

Then I realized this must be about just generally: any message nested within another message has this wire type.

So I was going to ask for more commentary based on my initial questions, but now I think this may be overemphasizing a point. Why is this more surprising than the fact that bools are encoded as varints? This seems to be one of any number of fiddly details, any of which we'd be kinda nuts to change, and any of which, if we did change them would fail a test.

I think this comment is only worth preserving if

• it was particularly hard to come by this knowledge (in which case I'd a link to the doc or sources that finally cleared this up)
• I'm misreading this and it actually is situational, in which case please tell more
• Changing this would survive the tests we have forthcoming and trigger undesirable behavior in our counterparty

[rsgowman]

I'll get rid of it. I added it for myself, as I found it mildly surprising that the wiretype for strings and submessages was the same. (Unlike the fact that a varint is used for bools, since I expect bools to be represented by ints.) But now that I've played around with nanopb a bit more, it seems less surprising now then it did before, since both strings and submessages are lengths followed by some arbitrary data.

[wilhuff]

This seems like it would be a good use of FIREBASE_DEV_ASSERT.

[rsgowman]

This would imply a corrupt message; I'm not sure an assertion properly applies since we should be able to detect that and return an error code rather than just failing an assertion or aborting. Either way, I'm going to defer this to the error handling PR (to come shortly).

[wilhuff]

This seems kind of oxymoronic because the reality is that outside of tests there never will be a top-level FieldValue. It seems strange to have to write two versions of EncodeFoo and EncodeNestedFoo just to have to make this work.

Meanwhile, it seems like this gunk about

• create sizing substream
• call some other code to actually write values
• take the size
• write the size
• if the stream is a sizing substream, inform it of the size
• otherwise
  • ensure space
  • new substream
  • call user code again to do it for real
  • adjust bytes written and propgate errors

If you took "user code" as a std::function that took a pb_ostream_t* then this whole function largely becomes an EncodeNestedMessage(pb_ostream_t* stream, std::function<void(pb_ostream_t*)>) and no longer specific to FieldValue at all.

Note: happy to address this during the second message type where you find yourself writing this triplet with different types.

[rsgowman]

Yeah, I like that approach. I'll do that as soon as we have a second message like this.

[wilhuff]

DecodeNested?

[rsgowman]

Done.

[wilhuff]

Actually there's nothing in here I think we need to change in this iteration. Let's refactor and iterate as we go.

LGTM