Parsing error / mapping issue

[EmergentOrder]

I'm trying to create a preset for ONNX. I've made pretty good progress and resolved a number of errors through using InfoMap. However, short of commenting out some lines in the header, I am still getting an error on parsing this header: https://github.com/onnx/onnx/blob/master/onnx/defs/schema.h
[note: line numbers below don't match those in the link above, there seems to be some reformatting happening along the way]

[INFO] Parsing /home/USER/javacpp-presets/onnx/cppbuild/linux-x86_64/include/defs/schema.h
[ERROR]Failed to execute JavaCPP Builder: /home/USER/javacpp-presets/onnx/cppbuild/linux-x86_64/include/defs/schema.h:454:Could not parse declaration at '::'

The offending line is:
std::map<std::string, Attribute> attributes_{};

Which I attempted to map like this:

.put(new Info("std::map<std::string,onnx::OpSchema::Attribute>").pointerTypes("StringAttributeMap").define())

When I comment out the offending line (and a few more referencing it), so I can get generated Java code, I see that StringAttributeMap is declared in the generated Java code. The class for Attribute also appears in the generated code, whether I explicitly map it or not. But I then get :

[ERROR] /home/USER/javacpp-presets/onnx/src/main/java/org/bytedeco/javacpp/onnx.java:[24,68] cannot find symbol
[ERROR] symbol:   class Attribute

[saudet]

Ah, yes, it's a limitation of the Parser when types like that get defined before the rest, but we can work around that with an Info like this:

.put(new Info("onnx::OpSchema::Attribute").pointerTypes("OpSchema.Attribute"))

[EmergentOrder]

Ok, now I have an error due to use of an (implicitly?) deleted function:

In file included from /home/USER/javacpp-presets/onnx/target/classes/org/bytedeco/javacpp/jnionnx.cpp:89:0:
/home/USER/javacpp-presets/onnx/cppbuild/linux-x86_64/include/defs/schema.h:287:10: note: ‘onnx::OpSchema::Attribute& onnx::OpSchema::Attribute::operator=(const onnx::OpSchema::Attribute&)’ is implicitly deleted because the default definition would be ill-formed:
   struct Attribute {

[EmergentOrder]

Also from the same run :

tuple:1172:70: error: no matching function for call to ‘onnx::OpSchema::Attribute::Attribute()’
         second(std::forward<_Args2>(std::get<_Indexes2>(__tuple2))...)

[saudet]

That would be a bug in ONNX. You'll need to patch it to correct that, or skip that declaration if you don't care about it.

[EmergentOrder]

hmm . but it compiles / links fine by itself, without javacpp, and also works with pybind11 (in the onnx repo). Also, are you saying the resolution to this should solve the original parsing error?

[saudet]

It won't compile if you try to use that std::map. If you don't use it, it builds fine.

[saudet]

Since we can't use it anyway, it's probably not important to map it, so just use a
new Info("onnx::OpSchema::attributes").skip() on it.

[saudet]

Still, could you point me to where the Python bindings for onnx::OpSchema::attributes() are? I'll take a look, and if there's something to fix in JavaCPP, I'll do that! Thanks

[EmergentOrder]

I don't think the bindings themselves are checked in to the repo, but you can generate them using https://github.com/onnx/onnx/blob/master/setup.py like so:

python setup.py install --single-version-externally-managed --record=record.txt

You can also install using

sudo apt-get install protobuf-compiler libprotoc-dev
pip install onnx

You can see an example usage of onnx::OpSchema::attributes() from Python here : https://github.com/onnx/onnx/blob/master/onnx/defs/gen_doc.py#L91

Skipping this is not an option for me, I'm specifically trying to get the operator schemas, including their attributes.

Thanks

[saudet]

Ok, thanks. So, let's see, would you also have code for JavaCPP somewhere that I could run right away here?

[EmergentOrder]

Sure, just checked it in to my fork here: https://github.com/EmergentOrder/javacpp-presets

[saudet]

Ok, so like I said, it's a bug in ONNX. The following doesn't compile:

#include <map>
#include "schema.h"

int main() {
    onnx::OpSchema s;
    const std::map<std::string, onnx::OpSchema::Attribute>& m = s.attributes(); 
    m["foo"];
}

How are we supposed to access the map?

[EmergentOrder]

Ok, I got OpSchema working, for my current needs. It looks like there are issues on both sides here.

On the ONNX side, as reported in the two errors above, we need to add a default constructor for Attribute, and a function was getting implicitly deleted( due to having const strings in a struct). Having fixed those two issues, the example you just gave compiles without a problem.

But, I still get the same parsing error from JavaCPP that I initially reported. I was able to fix this by changing

  std::map<std::string, Attribute> attributes_{};

to

  std::map<std::string, Attribute> attributes_;

in schema.h.
So there is something g++ doesn't take issue with but JavaCPP does. Turns out the issue wasn't the reported :: but instead it was {}

I've updated my fork so that it should work out of the box (without the parsing error), but you can edit /javacpp-presets/onnx/schema.h to reintroduce the curly brackets shown above to induce the parsing error.

[saudet]

Yes, that's a problem with JavaCPP. Fixed in the commit above.

[EmergentOrder]

Awesome, thanks!

[bddppq]

@saudet @EmergentOrder Attributes in a schema are read-only, that's why onnx::OpSchema::attributes function is returning a const ref to the internal attributes_ map. To access it you should do m.at("xxx") instead of m["xxx"], the latter can implicitly create a new entry in the map (which is not allowed).

[saudet]

I see, thanks for the explanation @bddppq! It seems to work fine on pretty much all C++ compilers that one would care about. It also looks like a good idea to use .at() on other containers like std::vector as well because it does bounds checking there, so I've switched in the commit above to calling that function by default instead of using operator[]. Sounds good @EmergentOrder?

[EmergentOrder]

thanks @saudet @bddppq !
This fixes one of the 3 errors I was trying to address with my PR to ONNX above.
I still get (on JavaCPP preset build, but not on vanilla ONNX build):

jnionnx.cpp:33289:43: error: use of deleted function ‘onnx::OpSchema::Attribute& onnx::OpSchema::Attribute::operator=(const onnx::OpSchema::Attribute&)’

which is because of implicit function deletion due to having const strings in a struct.

[EmergentOrder]

@saudet agree you should make the change for std::vector as well,I would think that would fix the default constructor issue I get for TypeConstraintParam just like it did for std::map / Attribute.
Currently though, I do still get the error on TypeConstraintParam:

jnionnx.cpp:32146:33:   required from here
/usr/include/c++/5/bits/stl_construct.h:75:7: error: no matching function for call to ‘onnx::OpSchema::TypeConstraintParam::TypeConstraintParam()’
     { ::new(static_cast<void*>(__p)) _T1(std::forward<_Args>(__args)...); }

[saudet]

How does ONNX itself copy Attribute then?

[saudet]

In any case, I thought about something, and with the commit above, we can now prevent defining the put() method with this kind of Info:

.put(new Info("const std::map<std::string,onnx::OpSchema::Attribute>").pointerTypes("StringAttributeMap").define())

[EmergentOrder]

Nice, your last commit fixes the issue with the const strings / Attribute map. Now I just have the TypeConstraintParam error.

[saudet]

For which declaration does that one happen?

[EmergentOrder]

std::vector<TypeConstraintParam> type_constraint_params_;

https://github.com/onnx/onnx/blob/v1.0.1/onnx/defs/schema.h#L455

[saudet]

Strange, it's working just fine for me with these Info:

.put(new Info("std::vector<std::string>").pointerTypes("StringVector").define())
.put(new Info("onnx::OpSchema::TypeConstraintParam").pointerTypes("OpSchema.TypeConstraintParam"))
.put(new Info("const std::vector<onnx::OpSchema::TypeConstraintParam>").pointerTypes("TypeConstraintParamVector").define())

Could you provide more details about the error you're getting?

[EmergentOrder]

@saudet ahh, I see. this time it was my mistake, I missed adding const to the TypeConstraintParamVector info. Works now, thanks for the reminder.
There are a couple of other one-line modifications I still have in place to make my preset build, but I'll look into them and open other tickets.
Closing this one, thanks for the help!