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

Remove ur::print namespace #1086

Closed
omarahmed1111 opened this issue Nov 16, 2023 · 9 comments
Closed

Remove ur::print namespace #1086

omarahmed1111 opened this issue Nov 16, 2023 · 9 comments
Labels
bug Something isn't working question Further information is requested

Comments

@omarahmed1111
Copy link
Contributor

While working on the third adapters branch bump PR. We been faced with a problem were this function was preventing the call for all ur::print overloaded print functions (this is after adding using namespace ur::print).

One of the solutions to this is to delete the ur::print namespace from ur_print.hpp file. We was wandering if this namespace have another purpose or why is it needed?

@omarahmed1111 omarahmed1111 added bug Something isn't working question Further information is requested conformance Conformance test suite issues. and removed conformance Conformance test suite issues. labels Nov 16, 2023
@omarahmed1111
Copy link
Contributor Author

Hi @PatKamin, What do you think of this?

@pbalcer
Copy link
Contributor

pbalcer commented Nov 16, 2023

The reason the ur::print namespace was added is to avoid polluting the global namespace with these overloads by merely including the ur_print.hpp file (which is now going to ship as part of UR).

This is unfortunately a compromise, because having the namespace makes using the ostream overloads a little bit more difficult as they are not picked up automatically for global C/plain-old-data structs.

Can you elaborate how the function you linked was preventing the call for all ur::print overloaded print functions? Maybe if using namespace ur::print is a problem, then a simpler solution would be to use a fully qualified call to the operator? Something like ur::print::operator<<(out, result.value).

@kbenzie
Copy link
Contributor

kbenzie commented Nov 16, 2023

The reason the ur::print namespace was added is to avoid polluting the global namespace with these overloads by merely including the ur_print.hpp file (which is now going to ship as part of UR).

This part is confusing me. How exactly is providing operator<<(ostream&, ...) overloads to types which exist in the global namespace polluting the global namespace?

@kbenzie
Copy link
Contributor

kbenzie commented Nov 16, 2023

If the ur_api.h types were in the ur namespace, providing the operator<<(ostream&, ...) overloads in that same namespace would be the recommended thing to do because argument-dependent name lookup would correctly resolve the desired overloads. Because ur_api.h is a C header, the types it defines are in the global namespace. As such, it is my believe that the operator<<(ostream&,...) overloads should exist in the same namespace as the C types, the global namespace, so that ADL works properly.

@pbalcer
Copy link
Contributor

pbalcer commented Nov 16, 2023

How exactly is providing operator<<(ostream&, ...) overloads to types which exist in the global namespace polluting the global namespace?

These overloads make choices on how the output is formatted, especially for structs and arrays. There might be cases where software might want to use a different implementation, while still having access to other ur::print functions. This is not a particularly strong argument (since we control all uses of this API at the moment and you can always define your own overload in a different scope), but I felt the trade-off worth it.

As such, it is my believe that the operator<<(ostream&,...) overloads should exist in the same namespace as the C types, the global namespace, so that ADL works properly.

Makes sense. I don't feel too strongly about this either way. If it makes things easier, I'm ok with removing the namespace.

@lukaszstolarczuk
Copy link
Contributor

@PatKamin, did your changes fix the issue? Should we close it now?

@PatKamin
Copy link
Contributor

@PatKamin, did your changes fix the issue? Should we close it now?

I think it's ok now. @omarahmed1111, can we close this issue?

@kbenzie
Copy link
Contributor

kbenzie commented Nov 30, 2023

Yeah, this can be closed.

@kbenzie kbenzie closed this as completed Nov 30, 2023
@kbenzie
Copy link
Contributor

kbenzie commented Nov 30, 2023

Thanks @PatKamin

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
bug Something isn't working question Further information is requested
Projects
None yet
Development

No branches or pull requests

5 participants