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

%feature and asterisks #2657

Open
mmomtchev opened this issue Jul 27, 2023 · 10 comments
Open

%feature and asterisks #2657

mmomtchev opened this issue Jul 27, 2023 · 10 comments

Comments

@mmomtchev
Copy link
Contributor

mmomtchev commented Jul 27, 2023

This is a valid %feature:

%feature("async", "0") *::Method;

However this is not:

%feature("async", "0") Klass::*;

(this seems to get parsed as a member pointer in the declarator item in the grammar)

This is valid too:

%feature("async", "0") Klass;

But in this case SWIG will make it available only to the class definition node (and the constructor) and not to the methods which still will get the global %feature. AFAIK, there isn't an easy way to check if the feature comes from the global features or from a local one to the symbol - which could make it possible to check the class feature when processing the class method.

Is this a bug, or simply an oversight/missing feature?

Alas, regexping is implemented only for renaming, maybe a more generalized version should apply to all features.

@mmomtchev
Copy link
Contributor Author

Maybe, as a first step, %feature("async", "0") Klass; should get propagated to all child nodes?

@mmomtchev
Copy link
Contributor Author

It is a bug:

/* A class-generic feature */

@mmomtchev
Copy link
Contributor Author

I have a fix in mmomtchev@d45b509 that depends on NAPI async, I will push it through after it is merged

@ojwb
Copy link
Member

ojwb commented Aug 2, 2023

Only a class wildcard is currently supported. I don't think Klass::* can be made to mean "any method of Klass" in SWIG because it already means "pointer to a member of Klass" in standard C++.

Allowing the match stuff from rename to work for %feature would provide a fairly nice way to achieve this without needing regexps - then you could use something like:

%feature("async", "0", %$classname="Klass");

@mmomtchev
Copy link
Contributor Author

Why not simply extend %feature("async", "0") Klass; to all members of Klass instead of only the class declaration and the constructor?

@ojwb
Copy link
Member

ojwb commented Aug 2, 2023

I left that part as I don't really know, but in a quick test it looks to me like the feature gets applied to the class and constructor because both match the specified name Klass. If you want only the class to match then you can use ::Klass and if you want only the constructor then Klass::Klass.

As to extending it as a generic change to how features work, I'd worry about breaking compatibility with usage of existing features which doesn't expect a feature applied to a class to get applied to class members implicitly, and there then doesn't seem an easy way to only apply a feature to a class (because there isn't a way to specify all members of a class...)

If you want these semantics for a particular new feature, you can probably just check the member for the feature and if it's not set there, check the parent class for it.

@mmomtchev
Copy link
Contributor Author

mmomtchev commented Aug 26, 2023

This would require to completely overhaul the features code because features are currently stored as a hash on the matching name, so having this:

%feature("async", "Async");
%feature("async", "0", %$classname="Klass");

The second directive simply overwrites the first one as both apply to the global context (an empty name).

The code expects this to apply to all class members:

%feature("async", "Async") Klass::;

However this statement violates the grammar. What about changing the grammar?

@mmomtchev
Copy link
Contributor Author

How about

%feature("async", "Async") Klass...;

The problem with Klass:: is that this is the beginning of a declarator so the grammar becomes impossible without backtracking - or making Klass:: a valid declarator which will have consequences for parsing C++.

@ojwb
Copy link
Member

ojwb commented Nov 20, 2023

It seems confusing to reuse ... and give it a new meaning unrelated to a variable number of arguments.

If we have to make up a syntax, we could perhaps use something like Klass::%any since SWIG already uses %foo-style identifiers quite heavily.

However I suspect we can make Klass:: work once we switch to using a GLR parser which is slated for early in 4.3.0 development. A GLR parser wouldn't need to backtrack because it tries all alternative parses in parallel, culling alternatives as they stop parsing successfully.

@mmomtchev
Copy link
Contributor Author

I can easily switch to %any but both my branch itself and the unit test depend on #2654

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

No branches or pull requests

2 participants