Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

AVRO-2271: C++ Support for Custom Fields #387

Open
wants to merge 3 commits into
base: main
Choose a base branch
from

Conversation

aniket486
Copy link
Contributor

No description provided.

@Fokko Fokko requested a review from thiru-mg November 20, 2018 09:16
@Fokko
Copy link
Contributor

Fokko commented Nov 20, 2018

Sad CI:

Scanning dependencies of target avrocpp_s
[  1%] Building CXX object CMakeFiles/avrocpp_s.dir/impl/Compiler.cc.o
/testptch/unknown/lang/c++/impl/Compiler.cc: In function ‘void avro::addCustomFields(const NodePtr&, const Object&)’:
/testptch/unknown/lang/c++/impl/Compiler.cc:299:53: error: in C++98 ‘kKnownFields’ must be initialized by constructor, not by ‘{...}’
          "values", "precision", "scale", "namespace"};
                                                     ^
/testptch/unknown/lang/c++/impl/Compiler.cc:299:53: error: could not convert ‘{"name", "type", "default", "doc", "size", "logicalType", "values", "precision", "scale", "namespace"}’ from ‘<brace-enclosed initializer list>’ to ‘const std::__debug::vector<std::basic_string<char>, std::allocator<std::basic_string<char> > >’
/testptch/unknown/lang/c++/impl/Compiler.cc:300:16: warning: ‘auto’ changes meaning in C++11; please remove it [-Wc++0x-compat]
     for (const auto &entry : m) {
                ^
/testptch/unknown/lang/c++/impl/Compiler.cc:300:22: error: ISO C++ forbids declaration of ‘entry’ with no type [-fpermissive]
     for (const auto &entry : m) {
                      ^
/testptch/unknown/lang/c++/impl/Compiler.cc:300:30: error: range-based ‘for’ loops are not allowed in C++98 mode
     for (const auto &entry : m) {
                              ^
/testptch/unknown/lang/c++/impl/Compiler.cc:301:71: error: request for member ‘first’ in ‘entry’, which is of non-class type ‘const int’
         if (std::find(kKnownFields.begin(), kKnownFields.end(), entry.first)
                                                                       ^
/testptch/unknown/lang/c++/impl/Compiler.cc:303:40: error: request for member ‘first’ in ‘entry’, which is of non-class type ‘const int’
             node->addCustomField(entry.first, entry.second);
                                        ^
/testptch/unknown/lang/c++/impl/Compiler.cc:303:53: error: request for member ‘second’ in ‘entry’, which is of non-class type ‘const int’
             node->addCustomField(entry.first, entry.second);
                                                     ^
make[2]: *** [CMakeFiles/avrocpp_s.dir/impl/Compiler.cc.o] Error 1
CMakeFiles/avrocpp_s.dir/build.make:54: recipe for target 'CMakeFiles/avrocpp_s.dir/impl/Compiler.cc.o' failed
make[1]: *** [CMakeFiles/avrocpp_s.dir/all] Error 2
CMakeFiles/Makefile2:425: recipe for target 'CMakeFiles/avrocpp_s.dir/all' failed
make: *** [all] Error 2
Makefile:147: recipe for target 'all' failed

@gioragutt
Copy link

You're using C++11 features, such as initializer_list and range-for, and seeing that the CI runs the code on C++98 (or just anything under 11) is problematic...

@thiru-mg
Copy link
Contributor

Instead of having a new class CustomFields can we not define a simple typedef?

    typedef std::map<std::string, json::Entity>

There is a little bit check done in addCustomField() member of the new class. That can be moved into its caller namely addCustomField() of Node.

At @gioragutt has mentioned, the code needs to compile under pre-2011 compilers.

@aniket486
Copy link
Contributor Author

Thanks. I have made changes to CMakeLists.txt to catch C++98 compat issues sooner during development process.

I have made suggested changes to the code. PTAL.

@iemejia iemejia added the C++ Pull Requests for C++ binding label Nov 29, 2018
@dkulp
Copy link
Member

dkulp commented Jan 3, 2019

Can you rebase this on master? Also, master is not c++11 so you can go back to using the c++11 things.

@thiru-mg
Copy link
Contributor

thiru-mg commented Jan 4, 2019

Apart from the fact that this needs to be rebased, a couple of issues:

  • Tests don't run
  • Please conform to the style of rest of code. For example, we don't use auto and instead use explicit type.
  • More serious problem. You include an implementation header (JsonDom.hh) in the publicly exported api headers. It is perfectly fine to include the .hh files from api in .cc or .hh files under impl, but not vice versa. When we install avro C++, only the files under api and the generated executable, .a and .so/.dll/.dylib files will be installed. The files under impl will NOT be installed. So including impl/json/JsonDom.hh in api will cause failures.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
C++ Pull Requests for C++ binding
Projects
None yet
6 participants