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

[UR] Fix some print operators not being exported #1091

Closed
wants to merge 1 commit into from

Conversation

PatKamin
Copy link
Contributor

No description provided.

@igchor
Copy link
Member

igchor commented Nov 17, 2023

Why should those operators be exported anyway? They are in the header so it shouldn't matter I think.

@PatKamin
Copy link
Contributor Author

Why should those operators be exported anyway? They are in the header so it shouldn't matter I think.

They are not visible in the loader's .lib file otherwise. Do we want them to be useful only with the headers?

@igchor
Copy link
Member

igchor commented Nov 20, 2023

They are not visible in the loader's .lib file otherwise.

But why is that a problem? Do we expect to need a function pointers to those operators or something like that? If not, what's the benefit of having those operators exported?

@PatKamin
Copy link
Contributor Author

But why is that a problem? Do we expect to need a function pointers to those operators or something like that? If not, what's the benefit of having those operators exported?

No, there is no need for function pointers to these operators, I suppose. I thought that these operators can't be used by an external binary by merely including the .hpp header and that these functions have to be exported in the library file as well (https://learn.microsoft.com/en-us/cpp/build/exporting-from-a-dll?view=msvc-170). However, I might understand it wrong and exporting shall be done only for the API functions included in the loader's function pointers table?

Shall all exports in this header be removed then? The exporting policy should be the same for all operators to keep the code clean.

@igchor
Copy link
Member

igchor commented Nov 20, 2023

But why is that a problem? Do we expect to need a function pointers to those operators or something like that? If not, what's the benefit of having those operators exported?

No, there is no need for function pointers to these operators, I suppose. I thought that these operators can't be used by an external binary by merely including the .hpp header and that these functions have to be exported in the library file as well (https://learn.microsoft.com/en-us/cpp/build/exporting-from-a-dll?view=msvc-170). However, I might understand it wrong and exporting shall be done only for the API functions included in the loader's function pointers table?

Shall all exports in this header be removed then? The exporting policy should be the same for all operators to keep the code clean.

I'm also not an expert on that but I found this answer which would suggest that if we don't need to take the address of such functions, exporting them is not necessary: https://stackoverflow.com/a/44183940/5935594 so yes, I would say that we should either remove exports for all functions that are defined in the header as inline or to define those functions in a .cpp file and export them.

Btw, why did we decide to implement those operators as inline in the header in the first place?

@PatKamin
Copy link
Contributor Author

But why is that a problem? Do we expect to need a function pointers to those operators or something like that? If not, what's the benefit of having those operators exported?

No, there is no need for function pointers to these operators, I suppose. I thought that these operators can't be used by an external binary by merely including the .hpp header and that these functions have to be exported in the library file as well (https://learn.microsoft.com/en-us/cpp/build/exporting-from-a-dll?view=msvc-170). However, I might understand it wrong and exporting shall be done only for the API functions included in the loader's function pointers table?
Shall all exports in this header be removed then? The exporting policy should be the same for all operators to keep the code clean.

I'm also not an expert on that but I found this answer which would suggest that if we don't need to take the address of such functions, exporting them is not necessary: https://stackoverflow.com/a/44183940/5935594 so yes, I would say that we should either remove exports for all functions that are defined in the header as inline or to define those functions in a .cpp file and export them.

Btw, why did we decide to implement those operators as inline in the header in the first place?

Right, based on these resources we can remove all exports from this file as it is not necessary to provide pointers to the operators. Inlining was introduced so that printing won't hurt performance, I suppose. I think that we want this to remain.

@pbalcer
Copy link
Contributor

pbalcer commented Nov 22, 2023

But why is that a problem? Do we expect to need a function pointers to those operators or something like that? If not, what's the benefit of having those operators exported?

No, there is no need for function pointers to these operators, I suppose. I thought that these operators can't be used by an external binary by merely including the .hpp header and that these functions have to be exported in the library file as well (https://learn.microsoft.com/en-us/cpp/build/exporting-from-a-dll?view=msvc-170). However, I might understand it wrong and exporting shall be done only for the API functions included in the loader's function pointers table?
Shall all exports in this header be removed then? The exporting policy should be the same for all operators to keep the code clean.

I'm also not an expert on that but I found this answer which would suggest that if we don't need to take the address of such functions, exporting them is not necessary: https://stackoverflow.com/a/44183940/5935594 so yes, I would say that we should either remove exports for all functions that are defined in the header as inline or to define those functions in a .cpp file and export them.
Btw, why did we decide to implement those operators as inline in the header in the first place?

Right, based on these resources we can remove all exports from this file as it is not necessary to provide pointers to the operators. Inlining was introduced so that printing won't hurt performance, I suppose. I think that we want this to remain.

Agreed. This is header only, so makes complete sense. Please close this PR and open a new one that removes the export markers.

@PatKamin
Copy link
Contributor Author

Changes in this PR are dropped and replaced with #1115

@PatKamin PatKamin closed this Nov 23, 2023
@PatKamin PatKamin deleted the fix-exports branch June 26, 2024 10:08
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.

3 participants