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

you are pulling 'std::variant' into the global namespace (bad idea) #108

Open
peterritter opened this issue Nov 2, 2023 · 2 comments
Open

Comments

@peterritter
Copy link

peterritter commented Nov 2, 2023

Hi

I have been struggling with some compiler errors involving 'variant'. I have my own 'variant' class in my library and it seemed to be clashing when I pulled in the tabulate header. I had a look and found this in tabulate.hpp:

#if __cplusplus >= 201703L
#include <string_view>
#include
using std::get_if;
using std::holds_alternative;
using std::string_view;
using std::variant;
using std::visit;

This is not a good practice to pull names from the standard library into the global namespace! Every library that uses tabulate will now have these names in the global namespace too.

I also found std::optional used this way.

I have manually gone and commented out all such using statements and added std:: prefix everywhere where they were used. It is not a lot of places - so it seems an easy fix!

Best Regards, P.

@p-ranav
Copy link
Owner

p-ranav commented Nov 3, 2023

Hi,

Thanks for the report. Would you be open to creating a PR? I agree with your comments here.

Regards,
Pranav

@peterritter
Copy link
Author

peterritter commented Nov 4, 2023

Hi,
I fixed it for myself in the 'single_include/tabulate.hpp' which is what I am using. I don't know what it looks like in the rest of the library. There may be other places that need fixing too in the non-amalgamated files. I also don't know what the interaction is with the 'nonstd' namespace - if that would create issues with that? I really just uncommented these and equivalent lines:

#if __cplusplus >= 201703L
#include <string_view>
#include
//using std::get_if;
//using std::holds_alternative;
//using std::string_view;
//using std::variant;
//using std::visit;
#else
// #include <tabulate/string_view_lite.hpp>
// #include <tabulate/variant_lite.hpp>
using nonstd::get_if;
using nonstd::holds_alternative;
using nonstd::string_view;
using nonstd::variant;
using nonstd::visit;
#endif

...and then added the 'std::' prefix where required.
Regards, Peter

PS: In practice, I realized that when populating large tables, it is not really practical to then iterate over the table again to set formating. It's really at the time of appending a cell to a row, that I would like to define the formating of the cell, as it is at that point that I know what the value type is. I tried quite hard to try to figure out how to format the cells in a row before appending a table. But i didn't quite manage to figure out how. If you ever get around to it that would be a good way to enhance the library.

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

No branches or pull requests

2 participants