AVRO-2397: Implement Alias Support for C++ #522
Conversation
…g to canonical form and trying decode again will FAIL as Aliases are stripped from canonical form.
| @@ -52,6 +53,8 @@ public: | |||
| void ns(const std::string& n) { ns_ = n; } | |||
| void simpleName(const std::string& n) { simpleName_ = n; } | |||
| void fullname(const std::string& n); | |||
| void addAlias(const std::string& alias); | |||
There was a problem hiding this comment.
It appears that you editor inserts hard-tab instead of spaces. We use four spaces for indentation.
| @@ -147,8 +150,8 @@ class AVRO_DECL Node : private boost::noncopyable | |||
| doAddName(name); | |||
| } | |||
| virtual size_t names() const = 0; | |||
| virtual const std::string &nameAt(int index) const = 0; | |||
| virtual bool nameIndex(const std::string &name, size_t &index) const = 0; | |||
| virtual const Name &nameAt(int index) const = 0; | |||
There was a problem hiding this comment.
This change will break existing client code that assigns a std::string with nameAt. One way to make it backward compatible is to have an implicit conversion from Name to std::string (as a member function in Name class.
| @@ -81,6 +81,25 @@ static bool isFullName(const string &s) | |||
| return s.find('.') != string::npos; | |||
| } | |||
|
|
|||
| static void addAliases(Name& nm, const Array * aliasesPtr) | |||
| { | |||
| if (aliasesPtr) | |||
There was a problem hiding this comment.
We use K&R convention of having { at the end of the previous line rather than on a line of its own. The only exception is standalone function. Even functions defined within a class definition use the end-of-line convention.
| { | ||
| string itemVal = item.stringValue(); | ||
| bool missingNs = nm.ns().empty(); | ||
| if(missingNs || isFullName(itemVal)) |
There was a problem hiding this comment.
As a coding style we use { and } even when we have just one statement. Also there should a space after if.
|
|
||
| bool Name::hasAlias() | ||
| { | ||
| return aliases_.size() > 0; |
There was a problem hiding this comment.
I think !aliases_.empty() is more compact a wee bit faster.
| const std::string& myFullName = fullname(); | ||
| const std::string& theirFullName = n.fullname(); | ||
|
|
||
| for (const auto& theirAlias : n.aliases_) |
There was a problem hiding this comment.
We have not used auto so far in our code base. I think it was because of some warning in some compilers that its meaning has changed or something like this. Is it possible to avoid using auto in these places before we figure out a consistent convention on use of auto?
| @@ -140,30 +140,30 @@ struct TestSchema | |||
| NodePtr node = schema_.root(); | |||
|
|
|||
| size_t index = 0; | |||
| bool found = node->nameIndex("mylongxxx", index); | |||
| bool found = node->nameIndex(std::string("mylongxxx"), index); | |||
There was a problem hiding this comment.
We typically use a convention when the same name from a namespace is used multiple times in .cc file: have a using declaration at the top and use the plain name.
|
Thanks for the prompt review. Once you have had a chance to look more and suggest any other changes, if you would like me to fixup the issues and resubmit a pull request let me know. This is my first time contributing to an open source project in Ernest so I am not exactly familiar with how things usually go. Do you typically just make the minor changes and merge them in or request submission of a fixed PR? |
Fokko
changed the title
This provided code here should implement alias support for the C++ implementation of AVRO.
You may or may not wish to include the CMake changes - they were required on my machine to build in VS2017 Windows.
Note: One of the parts of the edited tests still fails as it relies on converting the schema to canonical form and re-running resolution. As the canonical form removes aliases its expected to fail. I will leave it to the maintainers to decide how to handle that.
See bug entry here: https://issues.apache.org/jira/browse/AVRO-2397