Nested dictionaries for parsing aggregate data.

Nested dictionaries for parsing aggregate data.

R=roubert@google.com

Committed: https://code.google.com/p/libaddressinput/source/detail?r=223

[Rouslan Solomakhin, 2014-05-19 16:51:57]

Lara: PTAL Patch Set 1.

This patch enables parsing nested dictionaries, instead of flat dictionaries of
strings. This change enables parsing the aggregate data rules like
http://i18napis.appspot.com/ssl-aggregate-address/data/CH, for example.

[Rouslan Solomakhin, 2014-05-19 19:24:18]

Fredrik: I think this version does not have a leak, as it does not have a
memcpy() call.

[Rouslan Solomakhin, 2014-05-19 19:49:10]

Fredrik: Patch Set 3 should not have a leak, I promise :-)

[Rouslan Solomakhin, 2014-05-19 20:35:42]

Fredrik: PTAL Patch Set 4. All sub-documents are owned by the parent document.

[roubert (google), 2014-05-19 21:22:31]

https://codereview.appspot.com/97580043/diff/60002/cpp/src/util/json.cc
File cpp/src/util/json.cc (right):

https://codereview.appspot.com/97580043/diff/60002/cpp/src/util/json.cc#newco...
cpp/src/util/json.cc:34: using rapidjson::kParseValidateEncodingFlag;
The rest of this code base sorts lists like this ignoring case.

https://codereview.appspot.com/97580043/diff/60002/cpp/src/util/json.cc#newco...
cpp/src/util/json.cc:38: static JsonImpl* Owner(const Document* document) {
Instead of these builder functions, this would be a good moment to use
constructor polymorphism:

  explicit JsonImpl(const Document* document)
      : document_(document), value_(document), dictionaries_() {
    assert(value_ != NULL);
  }

  explicit JsonImpl(const Value* value)
      : document_(), value_(value), dictionaries_() {
    assert(value_ != NULL);
  }

With two constructors defined like that, the correct one will be automatically
selected depending on what data type is passed to it. That lowers the risk of
calling the wrong one.

https://codereview.appspot.com/97580043/diff/60002/cpp/src/util/json.cc#newco...
cpp/src/util/json.cc:42: static JsonImpl* WeakRef(const Value* document) {
s/document/value/

https://codereview.appspot.com/97580043/diff/60002/cpp/src/util/json.cc#newco...
cpp/src/util/json.cc:54: const Value& document() const { return *weak_; }
Please do a bit of name clean-up. It would be much more logical if a getter of
type Value was named value().

But as it's only used for calling IsObject() or FindMember(), it would be even
better if it was replaced by two helper methods with those names instead, that
just did those calls.

https://codereview.appspot.com/97580043/diff/60002/cpp/src/util/json.cc#newco...
cpp/src/util/json.cc:66: void AddSubDocument(const std::string& key, const Json*
sub_document) {
It would be better if you picked one term (eg. "dictionary") and used that term
everywhere.

https://codereview.appspot.com/97580043/diff/60002/cpp/src/util/json.cc#newco...
cpp/src/util/json.cc:67: assert(sub_documents_.insert(std::make_pair(key,
sub_document)).second);
You can't do work inside of assert(), as the compiler is free to optimize away
assert() statements entirely.

See MetadataLoader::RuleHierarchy::Load() for what you need to do in a situation
like this, first save the return value to a temporary variable, then assert()
that variable, then have a (void)tmp; statement so that the compiler won't see
that variable as unused after optimizing away the assert() statement.

https://codereview.appspot.com/97580043/diff/60002/cpp/src/util/json.cc#newco...
cpp/src/util/json.cc:78: // A Document object must always be deleted as its own
type, because neither
This comment seems to me to be misleading about what really is going on here.
When a JsonImpl object is constructed using a Document object, then JsonImpl is
supposed to take ownership of that object, making sure to delete it in its own
destructor. But when a JsonImpl object is constructed using a Value object, then
that object is owned by a Member object which is owned by a Document object, and
should therefore not be deleted by JsonImpl.

So even if there was a virtual destructor, that wouldn't change anything in your
code here, as you'd still have one kind of objects that you need to delete and
one kind of objects that you must not delete, and this is the important thing
here (while mentioning the virtual destructor just gives the reader information
overload).

https://codereview.appspot.com/97580043/diff/60002/cpp/src/util/json.cc#newco...
cpp/src/util/json.cc:88: std::map<std::string, const Json*> sub_documents_;
As this map is used to store the objects returned by the method
GetDictionaryValueForKey(), I'd suggest naming it dictionaries_.

https://codereview.appspot.com/97580043/diff/60002/cpp/src/util/json.cc#newco...
cpp/src/util/json.cc:140: impl_->document().FindMember(key.c_str());
You could actually cache this as well, by inserting a NULL pointer for this key
in the map if no such member exists. It's explicitly legal to call delete on
NULL pointers so you wouldn't have to change the destructor or anything else.

https://codereview.appspot.com/97580043/diff/60002/cpp/src/util/json.h
File cpp/src/util/json.h (right):

https://codereview.appspot.com/97580043/diff/60002/cpp/src/util/json.h#newcode59
cpp/src/util/json.h:59: // only as long as the this object is valid.
Hm, "the this object" is rather confusing. Maybe you don't need this sentence at
all. If a const& is returned, it ought to be pretty obvious that the object it
refers to is owned by the object returning it. (For where else could it possibly
come from?)

https://codereview.appspot.com/97580043/diff/60002/cpp/test/util/json_test.cc
File cpp/test/util/json_test.cc (right):

https://codereview.appspot.com/97580043/diff/60002/cpp/test/util/json_test.cc...
cpp/test/util/json_test.cc:17: #include <libaddressinput/util/scoped_ptr.h>
You don't actually use this.

[Rouslan Solomakhin, 2014-05-20 10:05:16]

Fredrik: PTAL Patch Set 5.

https://codereview.appspot.com/97580043/diff/60002/cpp/src/util/json.cc
File cpp/src/util/json.cc (right):

https://codereview.appspot.com/97580043/diff/60002/cpp/src/util/json.cc#newco...
cpp/src/util/json.cc:34: using rapidjson::kParseValidateEncodingFlag;
On 2014/05/19 21:22:30, roubert wrote:
> The rest of this code base sorts lists like this ignoring case.

Done.

https://codereview.appspot.com/97580043/diff/60002/cpp/src/util/json.cc#newco...
cpp/src/util/json.cc:38: static JsonImpl* Owner(const Document* document) {
On 2014/05/19 21:22:30, roubert wrote:
> Instead of these builder functions, this would be a good moment to use
> constructor polymorphism:
> 
>   explicit JsonImpl(const Document* document)
>       : document_(document), value_(document), dictionaries_() {
>     assert(value_ != NULL);
>   }
> 
>   explicit JsonImpl(const Value* value)
>       : document_(), value_(value), dictionaries_() {
>     assert(value_ != NULL);
>   }
> 
> With two constructors defined like that, the correct one will be automatically
> selected depending on what data type is passed to it. That lowers the risk of
> calling the wrong one.

Done.

https://codereview.appspot.com/97580043/diff/60002/cpp/src/util/json.cc#newco...
cpp/src/util/json.cc:42: static JsonImpl* WeakRef(const Value* document) {
On 2014/05/19 21:22:30, roubert wrote:
> s/document/value/

Done.

https://codereview.appspot.com/97580043/diff/60002/cpp/src/util/json.cc#newco...
cpp/src/util/json.cc:54: const Value& document() const { return *weak_; }
On 2014/05/19 21:22:30, roubert wrote:
> Please do a bit of name clean-up. It would be much more logical if a getter of
> type Value was named value().
> 
> But as it's only used for calling IsObject() or FindMember(), it would be even
> better if it was replaced by two helper methods with those names instead, that
> just did those calls.

Done.

https://codereview.appspot.com/97580043/diff/60002/cpp/src/util/json.cc#newco...
cpp/src/util/json.cc:66: void AddSubDocument(const std::string& key, const Json*
sub_document) {
On 2014/05/19 21:22:30, roubert wrote:
> It would be better if you picked one term (eg. "dictionary") and used that
term
> everywhere.

Done.

https://codereview.appspot.com/97580043/diff/60002/cpp/src/util/json.cc#newco...
cpp/src/util/json.cc:67: assert(sub_documents_.insert(std::make_pair(key,
sub_document)).second);
On 2014/05/19 21:22:30, roubert wrote:
> You can't do work inside of assert(), as the compiler is free to optimize away
> assert() statements entirely.
> 
> See MetadataLoader::RuleHierarchy::Load() for what you need to do in a
situation
> like this, first save the return value to a temporary variable, then assert()
> that variable, then have a (void)tmp; statement so that the compiler won't see
> that variable as unused after optimizing away the assert() statement.

Done.

https://codereview.appspot.com/97580043/diff/60002/cpp/src/util/json.cc#newco...
cpp/src/util/json.cc:78: // A Document object must always be deleted as its own
type, because neither
On 2014/05/19 21:22:30, roubert wrote:
> This comment seems to me to be misleading about what really is going on here.
> When a JsonImpl object is constructed using a Document object, then JsonImpl
is
> supposed to take ownership of that object, making sure to delete it in its own
> destructor. But when a JsonImpl object is constructed using a Value object,
then
> that object is owned by a Member object which is owned by a Document object,
and
> should therefore not be deleted by JsonImpl.
> 
> So even if there was a virtual destructor, that wouldn't change anything in
your
> code here, as you'd still have one kind of objects that you need to delete and
> one kind of objects that you must not delete, and this is the important thing
> here (while mentioning the virtual destructor just gives the reader
information
> overload).

Done.

https://codereview.appspot.com/97580043/diff/60002/cpp/src/util/json.cc#newco...
cpp/src/util/json.cc:88: std::map<std::string, const Json*> sub_documents_;
On 2014/05/19 21:22:30, roubert wrote:
> As this map is used to store the objects returned by the method
> GetDictionaryValueForKey(), I'd suggest naming it dictionaries_.

Done.

https://codereview.appspot.com/97580043/diff/60002/cpp/src/util/json.cc#newco...
cpp/src/util/json.cc:140: impl_->document().FindMember(key.c_str());
On 2014/05/19 21:22:30, roubert wrote:
> You could actually cache this as well, by inserting a NULL pointer for this
key
> in the map if no such member exists. It's explicitly legal to call delete on
> NULL pointers so you wouldn't have to change the destructor or anything else.

I think this would a non-trivial change, because I use
(impl_->FindSubDocument(key) != NULL) to determine if a dictionary is cached.

https://codereview.appspot.com/97580043/diff/60002/cpp/src/util/json.h
File cpp/src/util/json.h (right):

https://codereview.appspot.com/97580043/diff/60002/cpp/src/util/json.h#newcode59
cpp/src/util/json.h:59: // only as long as the this object is valid.
On 2014/05/19 21:22:30, roubert wrote:
> Hm, "the this object" is rather confusing. Maybe you don't need this sentence
at
> all. If a const& is returned, it ought to be pretty obvious that the object it
> refers to is owned by the object returning it. (For where else could it
possibly
> come from?)

Done.

https://codereview.appspot.com/97580043/diff/60002/cpp/test/util/json_test.cc
File cpp/test/util/json_test.cc (right):

https://codereview.appspot.com/97580043/diff/60002/cpp/test/util/json_test.cc...
cpp/test/util/json_test.cc:17: #include <libaddressinput/util/scoped_ptr.h>
On 2014/05/19 21:22:31, roubert wrote:
> You don't actually use this.

Done.

[roubert (google), 2014-05-20 11:41:03]

LGTM

https://codereview.appspot.com/97580043/diff/130001/cpp/src/util/json.cc
File cpp/src/util/json.cc (right):

https://codereview.appspot.com/97580043/diff/130001/cpp/src/util/json.cc#newc...
cpp/src/util/json.cc:83: // Avoid unusued variable warning when assert() is
optimized away.
s/unusued/unused/

https://codereview.appspot.com/97580043/diff/130001/cpp/src/util/json.cc#newc...
cpp/src/util/json.cc:98: // A weak reference to a JSON document. Cannot be NULL.
Can point to document_.
This line is too long.

https://codereview.appspot.com/97580043/diff/130001/cpp/src/util/json.cc#newc...
cpp/src/util/json.cc:98: // A weak reference to a JSON document. Cannot be NULL.
Can point to document_.
The expression "weak reference" is normally used to describe something else than
just a normal pointer to an object that someone else owns.

https://codereview.appspot.com/97580043/diff/130001/cpp/src/util/json.cc#newc...
cpp/src/util/json.cc:124: // Member is owned by impl.
s/impl/impl_/

https://codereview.appspot.com/97580043/diff/130001/cpp/src/util/json.cc#newc...
cpp/src/util/json.cc:133: // Member is owned by impl.
s/impl/impl_/

[Rouslan Solomakhin, 2014-05-20 11:49:24]

Will commit.

https://codereview.appspot.com/97580043/diff/130001/cpp/src/util/json.cc
File cpp/src/util/json.cc (right):

https://codereview.appspot.com/97580043/diff/130001/cpp/src/util/json.cc#newc...
cpp/src/util/json.cc:83: // Avoid unusued variable warning when assert() is
optimized away.
On 2014/05/20 11:41:03, roubert wrote:
> s/unusued/unused/

Done.

https://codereview.appspot.com/97580043/diff/130001/cpp/src/util/json.cc#newc...
cpp/src/util/json.cc:98: // A weak reference to a JSON document. Cannot be NULL.
Can point to document_.
On 2014/05/20 11:41:03, roubert wrote:
> The expression "weak reference" is normally used to describe something else
than
> just a normal pointer to an object that someone else owns.

Done.

https://codereview.appspot.com/97580043/diff/130001/cpp/src/util/json.cc#newc...
cpp/src/util/json.cc:98: // A weak reference to a JSON document. Cannot be NULL.
Can point to document_.
On 2014/05/20 11:41:03, roubert wrote:
> This line is too long.

Done.

https://codereview.appspot.com/97580043/diff/130001/cpp/src/util/json.cc#newc...
cpp/src/util/json.cc:124: // Member is owned by impl.
On 2014/05/20 11:41:03, roubert wrote:
> s/impl/impl_/

Done.

https://codereview.appspot.com/97580043/diff/130001/cpp/src/util/json.cc#newc...
cpp/src/util/json.cc:133: // Member is owned by impl.
On 2014/05/20 11:41:03, roubert wrote:
> s/impl/impl_/

Done.

[Rouslan Solomakhin, 2014-05-20 11:55:36]

Committed patchset #7 manually as r223 (presubmit successful).