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

* Fix InfoMap.normalize() incorrectly rebuilding nested templates #730

Closed
wants to merge 2 commits into from

Conversation

benshiffman
Copy link

When InfoMap.normalize() is called on "TemplatedClass<std::complex<double> >::funTemp<int>(const U&)", name is rebuilt as "TemplatedClass<std::complex<double>>::funTemp<int>(const U&)", making the parser fail to correctly handle templated functions which are members of templated classes.
This ensures that spacing is maintained between '>' characters when they close templates.

@HGuillemet
Copy link
Contributor

Thanks for tracking this down !

So the problem is that InfoMap.normalize doesn't normalize the same way when untemplate parameter is true or false, wrt spaces.

Since apparently normalize tries to normalize the spacing (at least before the function parameter list), I would add a " " instead of restoring token.spacing.

Your fix addresses the problem of nested template before the :: like in TemplatedClass<std::complex<double> >::funTemp but not the one for operators. Right ? So what about this:

if(i > 0 && (tokens[i].match('>') && tokens[i-1].match('>') || tokens[i-1].match(Token.OPERATOR))) {
  name += " ";
}

@benshiffman
Copy link
Author

benshiffman commented Dec 23, 2023 via email

@benshiffman
Copy link
Author

I'll admit I was in a manic headspace with that initial commit. Thank you for the nudge in the right direction. This gives some support to templated operator overloads but the overloads only respect the class templated type by default, not the function templated type. Also, javaNames aren't correctly applying for any templated functions in templated classes.

infoMap.put(new Info("TemplatedClass<double>::funTemp<int>(const U&)").javaNames("funTempInt"));
infoMap.put(new Info("TemplatedClass<std::complex<double> >::funTemp<int>(const U&)").javaNames("funTempInt"));
infoMap.put(new Info("TemplatedClass<double>::operator =<std::complex<double> >(const TemplatedClass<U>&)").javaNames("putConvert"));
infoMap.put(new Info("TemplatedClass<std::complex<double> >::operator =<double>(const TemplatedClass<U>&)").javaNames("putConvert"));

This will result in the following:

//TemplatedClassComplexDouble
public native void funTemp(int val);
public native @ByRef @Name("operator =") TemplatedClassComplexDouble put(@Const @ByRef TemplatedClassComplexDouble rhs);
//TemplatedClassDouble
public native void funTemp(int val);
public native @ByRef @Name("operator =") TemplatedClassDouble put(@Const @ByRef TemplatedClassDouble rhs);

If you have an idea of what the issue might be off the top of your head, another nudge would be appreciated but not expected. I'll keep cracking at this.

@HGuillemet
Copy link
Contributor

HGuillemet commented Dec 24, 2023

Concerning functions, you need apparently both this info to instantiate the function template:

infoMap.put(new Info("TemplatedClass<double>::funTemp<double>(const U&)").javaNames("funTempDouble"));

and this one to give it a name, since it's under this name (without <double>) that the infomap is queried when we generate the function instance:

infoMap.put(new Info("TemplatedClass<double>::funTemp(const double&)").javaNames("funTempDouble"));

This is not logical but at least we have a workaround.
If we want to improve this, I think we should add another infomap query, similar to fullname and fullname2 (fullname3 ?) that includes the template argument (available from context.templateMap).

Concerning operators, I see that the template arguments after operator XX are simply not parsed in type(). I'll fix that in a separate PR.

@benshiffman
Copy link
Author

Concerning functions, you need apparently both this info to instantiate the function template:

infoMap.put(new Info("TemplatedClass<double>::funTemp<double>(const U&)").javaNames("funTempDouble"));

and this one to give it a name, since it's under this name (without <double>) that the infomap is queried when we generate the function instance:

infoMap.put(new Info("TemplatedClass<double>::funTemp(const double&)").javaNames("funTempDouble"));

I tested this and interestingly as long as the javaNames in the first Info is a non-empty string, the function will be renamed with the second javaNames.

This is not logical but at least we have a workaround. If we want to improve this, I think we should add another infomap query, similar to fullname and fullname2 (fullname3 ?) that includes the template argument (available from context.templateMap).

I will look into this next week. Seems fairly straightforward given I'm now more familiar with that portion of the Parser especially.

Concerning operators, I see that the template arguments after operator XX are simply not parsed in type(). I'll fix that in a separate PR.

Thank you!

@saudet
Copy link
Member

saudet commented Dec 25, 2023

@HGuillemet Please approve this if it looks good to merge!

@HGuillemet
Copy link
Contributor

This fix doesn't solve all issues with template functions and operators. I'm working on another PR that hopefully should. It will use the methods inTemplates to untemplate in InfoMap.normalize.
Please hold on a day or two.

@HGuillemet
Copy link
Contributor

That turned out to be a more complicated that expected. PR #732 created.

@saudet
Copy link
Member

saudet commented Dec 31, 2023

Duplicate of #732

@saudet saudet marked this as a duplicate of #732 Dec 31, 2023
@saudet saudet closed this Dec 31, 2023
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
Development

Successfully merging this pull request may close these issues.

None yet

3 participants