UserDataReader#7843
Conversation
Generated by 🚫 Danger |
Coverage ReportAffected SDKs
Test Logs |
| input = self.preConverter(input); | ||
| if ([input isKindOfClass:[NSDictionary class]]) { | ||
| return [self parseDictionary:(NSDictionary *)input context:std::move(context)]; | ||
| return absl::optional<google_firestore_v1_Value>{[self parseDictionary:(NSDictionary *)input |
There was a problem hiding this comment.
Nit: you should be able to just return [self parseDictionary] (the way it was done before) -- an optional can be implicitly created from the value (here and below).
There was a problem hiding this comment.
Fixed here and everywhere. It shows up as a compile error in CLion, but the build passes.
There was a problem hiding this comment.
Just out of curiosity, what is the error?
There was a problem hiding this comment.
Returning google_firestore_v1_Value from a method returning absl::optional<google_firestore_v1_Value>: Class google_firestore_v1_Value is not compatible with absl::optional<google_firestore_v1_Value>
| // Populate a vector of fields since we don't know the size of the final fields array. | ||
| [dict enumerateKeysAndObjectsUsingBlock:^(NSString *key, id value, BOOL *) { | ||
| absl::optional<FieldValue> parsedValue = | ||
| absl::optional<google_firestore_v1_Value> parsedValue = |
There was a problem hiding this comment.
Question: what does it take to know whether the value can be parsed? What I'm getting at is whether it could be feasible to do a first pass on dict to get the array size, then create the values right in the array and skip the vector.
There was a problem hiding this comment.
We have to check whether it is a FIeldValue sentinel, which would duplicate the existing check in parseData. I don't think we can remove the parseData check since this function has more than this callsite.
There was a problem hiding this comment.
Ok, if I'm reading the code correctly, it amounts to going through the dictionary and calling [input isKindOfClass:[FIRFieldValue class]] on each element. If this is correct, then I strongly suspect that it's faster than creating and filling a vector and then copying vector elements into the array. What do you think about doing this, perhaps in a follow-up?
There was a problem hiding this comment.
I updated this in this PR.
* Transfer master branch testing podspec repo to open source. * Update the tool to only remove dirs and retain CONTRIBUTION, LICENSE and README.md.
var-const
left a comment
var-const
left a comment
There was a problem hiding this comment.
Pretty much LGTM with one question.
| input = self.preConverter(input); | ||
| if ([input isKindOfClass:[NSDictionary class]]) { | ||
| return [self parseDictionary:(NSDictionary *)input context:std::move(context)]; | ||
| return absl::optional<google_firestore_v1_Value>{[self parseDictionary:(NSDictionary *)input |
There was a problem hiding this comment.
Just out of curiosity, what is the error?
| // Populate a vector of fields since we don't know the size of the final fields array. | ||
| [dict enumerateKeysAndObjectsUsingBlock:^(NSString *key, id value, BOOL *) { | ||
| absl::optional<FieldValue> parsedValue = | ||
| absl::optional<google_firestore_v1_Value> parsedValue = |
There was a problem hiding this comment.
Ok, if I'm reading the code correctly, it amounts to going through the dictionary and calling [input isKindOfClass:[FIRFieldValue class]] on each element. If this is correct, then I strongly suspect that it's faster than creating and filling a vector and then copying vector elements into the array. What do you think about doing this, perhaps in a follow-up?
schmidt-sebastian
force-pushed
the
mrschmidt/userdatareader
branch
from
abe6480 to
5c1e66f
Compare
5 participants
Note: This PR is much smaller than it appears.
This adds Android's UserDataReader, which is basically UserDataConverter but instead of converting user data to FieldValues, it directly converts user data to Proto. To make this less intrusive, UserDataReader currently uses UserDataConverter to then convert the Protos to FieldValues (this step will be removed soon).
To make the diff easier to review, UserDataConverter is renamed to UserDataConverter_legacy and a bunch of comment changes have been made to force Git to show the diff between UserDataReader and UserDataConverter (rather than from UserDataConverter to UserDataConverter_legacy).