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 "using namespace std;" from XRootD (necessary to support C++17) #1933

Closed
wants to merge 1 commit into from

Conversation

amadio
Copy link
Member

@amadio amadio commented Feb 24, 2023

This is necessary to avoid clashes of names from the global namespace with names from the std namespace. It was identified as a problem when trying to compile XRootD with C++17 standard and support for VOMS, which has a 'struct data' in the global namespace that clashes with std::data from C++17.

What I'm looking at for review is if using std:: everywhere is fine or if there's a preference to add using namespace std; in the .cc files. I think adding std:: explicitly is more reliable, but the patch is quite large. Once we merge this, #1929 should be fine to merge as well without problems.

@amadio
Copy link
Member Author

amadio commented Mar 23, 2023

Actually, I think I will split this into two changes. One patch to not need using namespace std; in our code, which can be merged into master without causing problems downstream, and another that just removes the using namespace std; lines from our headers, which should only be merged when we do the next major release. What do you think, @abh3?

@amadio
Copy link
Member Author

amadio commented Mar 23, 2023

Alright, the change is done. The first commit will be pushed to master, while the second has to wait for the major release.

@xrootd-dev
Copy link

xrootd-dev commented Mar 23, 2023 via email

@amadio amadio force-pushed the cpp17 branch 2 times, most recently from 4e46ff0 to 020282f Compare April 5, 2023 08:00
@amadio amadio changed the title Stop using namespace std (necessary to support C++17) Remove "using namespace std;" from XRootD (necessary to support C++17) Apr 5, 2023
@amadio
Copy link
Member Author

amadio commented Apr 5, 2023

Ok, I've pushed the commit that changes the code not to rely on using namespace std; to master, so now we only need to add later the change that actually removes the lines with using namespace std; from our headers, which is what remains in this pull request and is actually the part that may break downstream code. This will only go in for the next major version.

This is necessary to avoid name clashes between the std and global
namespaces. It was identified as a problem when trying to compile XRootD
with C++17 standard and support for VOMS enabled, since VOMS has a struct
named 'struct data' in the global namespace, which clashes with std::data
from C++17.
Copy link
Member

@abh3 abh3 left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

OK, all I see is using std removals. I also see that implicit use of std was already corrected in a previous pull request. So, it looks OK. I was wondering why did we eschew using declaration notation to fix this?

@amadio
Copy link
Member Author

amadio commented Apr 17, 2023

OK, all I see is using std removals. I also see that implicit use of std was already corrected in a previous pull request. So, it looks OK. I was wondering why did we eschew using declaration notation to fix this?

This is the same pull request, but I separated the parts fixing implicit usage of std and pushed it, so all that's left here is the removal of using namespace std; from the code.

As for using declarations, in one of our meetings when we discussed this, we decided to keep using std:: in code rather than declarations for the sake of consistency. Since we cannot use declarations in our headers, rather than have a separate policy for headers and sources, we decided to go with using std:: everywhere.

@abh3
Copy link
Member

abh3 commented Apr 17, 2023

Yes, I do remember we decided on that. I was just having second thoughts given what other projects do. For instance, in LSST common std symbols are declared in the cc file in which they are used (e.g. std::vector, std::string, etc) to shorten the code. The idea here is that these names have now become effectively baked into the compiler. So, that was all I was going after to see what you thought about it.

BTW if you are waiting for Michal to do a review you may never stop waiting. I think the rest of the month he's using up his vacation. So, you may want to remove him as a reviewer and merge this.

@amadio amadio removed the request for review from simonmichal April 17, 2023 16:10
@amadio
Copy link
Member Author

amadio commented Apr 17, 2023

This has to wait for 6.0 to be merged, so I will close for now and keep the branch on my fork to avoid premature merging by accident. I will open a new PR when the right time arrives.

@amadio amadio closed this Apr 17, 2023
@amadio amadio removed this from the 6.0 milestone Apr 17, 2023
@amadio amadio deleted the cpp17 branch April 18, 2023 07:13
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.

None yet

3 participants