grasph / wrapit

Automatization of C++--Julia wrapper generation
MIT License
98 stars 12 forks source link

Recognize `jlcxx::Array` and `jlcxx::ArrayRef` as natively supported #60

Closed JamesWrigley closed 2 weeks ago

JamesWrigley commented 2 weeks ago

This is a speculative attempt to add an override to the auto_veto feature. It's useful in cases where there are many functions that shouldn't be wrapped that are correctly vetoed by auto_veto, but there's a few that should be wrapped anyway.

My motivation is that there there's a function I'm defining in a custom header that returns a jlcxx::Array. CxxWrap already supports this so nothing extra is required, but wrapit will create wrappers for jlcxx::Array anyway and insert them into the module. I don't want this to happen so I tried adding jlcxx::Array to the veto list, but because of auto_veto my function was removed as well. And I don't want to disable auto_veto because there are many dozens of functions that it correctly removes. With this PR I can set veto_overrides_list = "veto_overrides.txt" and allow only that one function to get through auto_veto.

An alternative approach would be to add built-in support for the jlcxx::Array/jlcxx::ArrayRef types, but I figured this is more general so it might be more useful. What do you think?

grasph commented 2 weeks ago

I think there should be a builtin support for these types see CodeTree.cpp, lin 2917 where types natively supported by CxxWrap are specified.

It does not prevent to have an override veto feature. For this, instead of adding an extra file, I would add the convention that a line in the veto file starting with ! will have the effect to unveto, overriding vetoes preceding it in the file: to check if a type or function is vetoed, we parse the file starting from the top to bottom, when it matches with a veto line, we set its veto state to true, when it matches with an unveto line, the state is set to false. In addition to override auto_veto, the feature can be used to tune a veto defined with a regex pattern. In such case, I think it is clearer if both vetoes and veto-overrides are specified in the same file.

There is a plan to generate a project database in the form of an editable toml file: see src/TODO.md. The database will allow overriding an auto_veto.

JamesWrigley commented 2 weeks ago

I think there should be a builtin support for these types see CodeTree.cpp, lin 2917 where types natively supported by CxxWrap are specified.

Ahhh, that is exactly what I needed :upside_down_face: I replaced all the commits with 128e622 to add support for jlcxx::Array and jlcxx::ArrayRef.

It does not prevent to have an override veto feature. For this, instead of adding an extra file, I would add the convention that a line in the veto file starting with ! will have the effect to unveto, overriding vetoes preceding it in the file: to check if a type or function is vetoed, we parse the file starting from the top to bottom, when it matches with a veto line, we set its veto state to true, when it matches with an unveto line, the state is set to false. In addition to override auto_veto, the feature can be used to tune a veto defined with a regex pattern. In such case, I think it is clearer if both vetoes and veto-overrides are specified in the same file.

Yes, that makes sense :+1: I'll leave that for another PR if I need it.