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

Narrow down context.virtualize at function level #658

Merged
merged 5 commits into from
Mar 21, 2023

Conversation

HGuillemet
Copy link
Contributor

See #656

@saudet
Copy link
Member

saudet commented Mar 16, 2023

Could you test this with all (non-obsolete) presets that use Info.virtualize() just to make sure nothing breaks?

@HGuillemet HGuillemet closed this Mar 16, 2023
@HGuillemet HGuillemet deleted the virtualizeèfunction_level branch March 16, 2023 22:26
@HGuillemet HGuillemet restored the virtualizeèfunction_level branch March 16, 2023 22:33
@HGuillemet HGuillemet reopened this Mar 16, 2023
@HGuillemet
Copy link
Contributor Author

When the override specifier is present, the member function is virtual, even if the virtual specifier is not present.
But the declarator is parsed before the override keyword, so we don't know the function is marked override yet, so we can fail to annotate the parameters correctly.
We could probably move the override parsing from here to somewhere around here.

But the override keyword is optional and a function overriding a virtual function is virtual, even if not marked virtual nor override. Right ? So if we want to solve the issue rigorously, we would need to detect the actual overriding. This logic doesn't exist in Parser (but it does in Generator).

What do you think about this ?
This is not strictly related to this PR. What made me think about it is that the PR now ignores the override keyword when checking if @Virtual must be added, compared to master. I'll fix this, but how to do it depends on your answer about the issue above.

@saudet
Copy link
Member

saudet commented Mar 18, 2023

Well that identifier only appears after the parameters, so the easiest way to fix this might be to call function() again recursively?

@HGuillemet
Copy link
Contributor Author

I didn't see an easy way to call function recursively, and since the declaration is already parsed a first time before the for (int n = -2; n < Integer.MAX_VALUE; n++) loop but stops after the parameters, I just pushed it a little further to include the post-parameter specifiers.

I re-checked all presets with this new commit.

No change in any parsing result but:

caffe: a parameter of non-virtual functions of virtualized class have lost their @Const annotation:

  • constructors of *Solver (DoubleAdaDeltaSolver, DoubleAdamSolver...)
  • loss, set_loss, param_propagate_down, set_param_propagate_down in DoubleLayer and FloatLayer.
    JNI compiles.

I guess the new behavior is correct ?

qt: same for QByteArray.fromStdString, QString.fromStdString and QString.fromStdWString

not tested: arpack-ng (could not compile with CentOS docker), skia (could not download)

@saudet
Copy link
Member

saudet commented Mar 19, 2023

What are you trying to do exactly here? I don't see how that is supposed to pick up override after the parameters.

@HGuillemet
Copy link
Contributor Author

What are you trying to do exactly here? I don't see how that is supposed to pick up override after the parameters.

postDeclarator returns a modified Type with virtual set if override is present. What makes you say it doesn't ?

The aim is to set context.virtual when override is present, not only when virtual is present, so that:

  • when declarator is called in the for (int n... loop the proper annotations are set on parameters for virtualization
  • the @Virtual annotation is added.

In master, context.virtual is true for all functions of a virtualized class, so the parameter annotations is always added (which caused the bug with Module.name() in Pytorch when Module is virtualized), and @Virtual is added correctly.

@HGuillemet
Copy link
Contributor Author

qt: same for QByteArray.fromStdString, QString.fromStdString and QString.fromStdWString

BTW, why is there a virtualize info for QString ? I don't see anything virtual there.

@saudet saudet merged commit ea4e5f7 into bytedeco:master Mar 21, 2023
@HGuillemet HGuillemet deleted the virtualizeèfunction_level branch March 21, 2023 16:37
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.

2 participants