bytedeco / javacpp

The missing bridge between Java and native C++
Other
4.43k stars 576 forks source link

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

Closed benshiffman closed 6 months ago

benshiffman commented 7 months ago

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 commented 7 months ago

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 commented 7 months ago

That’s a great point, and might actually kill a second bird with this same stone. As you might recall I discovered that templated operators in templated classes were not able to map with “.javaNames()”. It would be great if this were the solution to that problem as well.

El El sáb, 23 dic 2023 a las 3:33 a. m., HGuillemet < @.***> escribió:

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

::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 += " "; }

— Reply to this email directly, view it on GitHub https://github.com/bytedeco/javacpp/pull/730#issuecomment-1868274653, or unsubscribe https://github.com/notifications/unsubscribe-auth/AFC2WFJCPKBUVDXRMGNKB6LYK26PXAVCNFSM6AAAAABBAOO7G2VHI2DSMVQWIX3LMV43OSLTON2WKQ3PNVWWK3TUHMYTQNRYGI3TINRVGM . You are receiving this because you authored the thread.Message ID: @.***>

benshiffman commented 6 months ago

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 commented 6 months ago

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 commented 6 months ago

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 commented 6 months ago

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

HGuillemet commented 6 months ago

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 commented 6 months ago

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

saudet commented 6 months ago

Duplicate of #732