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

Linking fails when calling setUniform(string, Mat4) #1044

Closed
TankOs opened this Issue Jan 20, 2016 · 11 comments

Comments

Projects
None yet
4 participants
@TankOs
Member

TankOs commented Jan 20, 2016

Reported here, incl. example code: http://en.sfml-dev.org/forums/index.php?topic=19677

The reason for failed linking is that setUniform(string, Mat4) is a template and calls the non-exported function copyMatrix() in Glsl.inl. Linking works when prepending copyMatrix() by SFML_GRAPHICS_API; this also applies to other copy* functions in that header.

@Bromeon Mentioning you here because you wrote the code. I'd contribute a patch if you don't have anything else in mind. Also, what's your opinion on moving the copy* functions to the .hpp file instead? They are no templates anyway.

@TankOs TankOs added this to the 2.4 milestone Jan 20, 2016

@Bromeon

This comment has been minimized.

Show comment
Hide comment
@Bromeon

Bromeon Jan 21, 2016

Member

Also, what's your opinion on moving the copy* functions to the .hpp file instead? They are no templates anyway.

I put everything I could in the .inl file to keep the .hpp header as clean as possible (including the Matrix and Vector class definitions). I'm not sure if it's better when the .hpp begins with a huge block of implementation details (i.e. namespace priv), it makes it more difficult to get a quick overview.

Yes, let's add the SFML_GRAPHICS_API macros. I don't care who does it, I can also fix it (tomorrow) :)

Member

Bromeon commented Jan 21, 2016

Also, what's your opinion on moving the copy* functions to the .hpp file instead? They are no templates anyway.

I put everything I could in the .inl file to keep the .hpp header as clean as possible (including the Matrix and Vector class definitions). I'm not sure if it's better when the .hpp begins with a huge block of implementation details (i.e. namespace priv), it makes it more difficult to get a quick overview.

Yes, let's add the SFML_GRAPHICS_API macros. I don't care who does it, I can also fix it (tomorrow) :)

@eXpl0it3r

This comment has been minimized.

Show comment
Hide comment
@eXpl0it3r

eXpl0it3r Jan 22, 2016

Member

Since when are the *.inl files there to "hide" code? 😕

Member

eXpl0it3r commented Jan 22, 2016

Since when are the *.inl files there to "hide" code? 😕

@Bromeon

This comment has been minimized.

Show comment
Hide comment
@Bromeon

Bromeon Jan 22, 2016

Member

Since when are the *.inl files there to "hide" code? 😕

November 20, 2010

Member

Bromeon commented Jan 22, 2016

Since when are the *.inl files there to "hide" code? 😕

November 20, 2010

@TankOs

This comment has been minimized.

Show comment
Hide comment
@TankOs

TankOs Jan 22, 2016

Member

I can understand the case in Thread.inl, as 98% of the code are templates (except the base class, which is pure virtual). The copy functions are not "closely" related to templates at all, though.

It was just a suggestion anyway. I gonna add the macros and leave the rest as it is. ;)

Member

TankOs commented Jan 22, 2016

I can understand the case in Thread.inl, as 98% of the code are templates (except the base class, which is pure virtual). The copy functions are not "closely" related to templates at all, though.

It was just a suggestion anyway. I gonna add the macros and leave the rest as it is. ;)

@LaurentGomila

This comment has been minimized.

Show comment
Hide comment
@LaurentGomila

LaurentGomila Jan 22, 2016

Member

The copy functions are implementation details, and thus I think they should be hidden as much as possible. There's no benefit to expose them in the header.

Member

LaurentGomila commented Jan 22, 2016

The copy functions are implementation details, and thus I think they should be hidden as much as possible. There's no benefit to expose them in the header.

@TankOs

This comment has been minimized.

Show comment
Hide comment
@TankOs

TankOs Jan 22, 2016

Member

I agree that they are not important for the user at all, and if it was possible, they would completely disappear from any public header. But since that doesn't work due to template code, it can't be "hidden" because it's being used by the user, indirectly, and the .inl file is included anyway.

I was confused at first because I found function declarations (!) in an .inl file. The general way of using .hpp/.inl is using .hpp for declarations, and .inl for definitions, just like you'd do with .hpp/.cpp — and you only do that because there's no other way (template code). That's why I wondered that the .inl contains declarations which are defined in a .cpp file.

Personally I think that hiding as much as possible is of course good. But moving away from the widely agreed-on .hpp/.inl behavior can at least lead to some question marks over some people's heads. ;-)

As I stated before, I'm not going to change this, and probably also not going into a debate about it. It's no big deal..

Member

TankOs commented Jan 22, 2016

I agree that they are not important for the user at all, and if it was possible, they would completely disappear from any public header. But since that doesn't work due to template code, it can't be "hidden" because it's being used by the user, indirectly, and the .inl file is included anyway.

I was confused at first because I found function declarations (!) in an .inl file. The general way of using .hpp/.inl is using .hpp for declarations, and .inl for definitions, just like you'd do with .hpp/.cpp — and you only do that because there's no other way (template code). That's why I wondered that the .inl contains declarations which are defined in a .cpp file.

Personally I think that hiding as much as possible is of course good. But moving away from the widely agreed-on .hpp/.inl behavior can at least lead to some question marks over some people's heads. ;-)

As I stated before, I'm not going to change this, and probably also not going into a debate about it. It's no big deal..

@LaurentGomila

This comment has been minimized.

Show comment
Hide comment
@LaurentGomila

LaurentGomila Jan 22, 2016

Member

just like you'd do with .hpp/.cpp

We put all private stuff into .cpp as much as possible (in anonymous namespaces). In this specific case, if the .inl file was a .cpp, I wouldn't change anything. To me, the current code is perfectly consistent with our habits.

Member

LaurentGomila commented Jan 22, 2016

just like you'd do with .hpp/.cpp

We put all private stuff into .cpp as much as possible (in anonymous namespaces). In this specific case, if the .inl file was a .cpp, I wouldn't change anything. To me, the current code is perfectly consistent with our habits.

@TankOs

This comment has been minimized.

Show comment
Hide comment
@TankOs

TankOs Jan 22, 2016

Member

If the .inl file was a .cpp, you'd have put the declaration in it.

Member

TankOs commented Jan 22, 2016

If the .inl file was a .cpp, you'd have put the declaration in it.

@LaurentGomila

This comment has been minimized.

Show comment
Hide comment
@LaurentGomila

LaurentGomila Jan 22, 2016

Member

Which declaration?

Member

LaurentGomila commented Jan 22, 2016

Which declaration?

@TankOs

This comment has been minimized.

Show comment
Hide comment
@TankOs

TankOs Jan 22, 2016

Member

void copyMatrix(...);. It's declared in .inl, and implemented in .cpp.

Member

TankOs commented Jan 22, 2016

void copyMatrix(...);. It's declared in .inl, and implemented in .cpp.

@eXpl0it3r

This comment has been minimized.

Show comment
Hide comment
@eXpl0it3r

eXpl0it3r Mar 1, 2016

Member

Merged in b61502b

Member

eXpl0it3r commented Mar 1, 2016

Merged in b61502b

@eXpl0it3r eXpl0it3r closed this Mar 1, 2016

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment