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

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

Merged
merged 1 commit into from
Jun 20, 2024

Conversation

JamesWrigley
Copy link
Contributor

@JamesWrigley JamesWrigley commented Jun 19, 2024

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
Copy link
Owner

grasph commented Jun 19, 2024

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
Copy link
Contributor Author

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 🙃 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 👍 I'll leave that for another PR if I need it.

@JamesWrigley JamesWrigley changed the title Add support for a veto_overrides_list to explicitly allow symbols Recognize jlcxx::Array and jlcxx::ArrayRef as natively supported Jun 20, 2024
@grasph grasph merged commit 1bd07c7 into grasph:main Jun 20, 2024
4 checks passed
@JamesWrigley JamesWrigley deleted the veto_overrides branch June 20, 2024 11:52
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

None yet

2 participants