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

[BUG] Remove using namespace from header files #2630

Open
mdemoret-nv opened this issue Jul 31, 2020 · 7 comments
Open

[BUG] Remove using namespace from header files #2630

mdemoret-nv opened this issue Jul 31, 2020 · 7 comments
Labels
? - Needs Triage Need team to review and classify bug Something isn't working CUDA / C++ CUDA issue inactive-90d Tech Debt Issues related to debt

Comments

@mdemoret-nv
Copy link
Contributor

Ran into an issue where a using namespace directive caused a build failure due to a collision between MLCommon::swap() and std::swap. A quick search of header files returns 38 hits for using namespace in any .cuh, .h or .hpp file. These should be removed from header files where possible, especially if the statement is not in a namespace directive since this will pollute the global namespace. There may only be a few that are in the global namespace, but it should be audited and fixed to prevent future issues.

One example of this is here: https://github.com/rapidsai/cuml/blob/branch-0.15/cpp/src/dbscan/adjgraph/algo.cuh#L29.

@mdemoret-nv mdemoret-nv added ? - Needs Triage Need team to review and classify bug Something isn't working labels Jul 31, 2020
@mdemoret-nv mdemoret-nv added CUDA / C++ CUDA issue Tech Debt Issues related to debt labels Jul 31, 2020
@drobison00
Copy link
Contributor

@mdemoret-nv If this is a significant problem, and we don't want 'using' directives, this probably needs a corresponding feature request to update the style checking code.

@mdemoret-nv
Copy link
Contributor Author

I dont know if this is a "significant" problem since we have been building and developing without a style checker for a while. This bug could be broken up into 2 parts: finding and fixing existing issues (bug) and preventing future ones (fea). The amount of work really is going to depend on how strict we want to be. We need to answer the following questions first:

  1. Do we allow using namespace ...; if it's in a namespace?
  2. Are there specific ones we don't allow even within a namespace
    1. I think using namespace std; should never be used but thats just my opinion
  3. Or, do we get rid of all using statements in headers across the board?

I think there are very few using namespace ...; in the global scope in our code. The majority seem to be within namespaces which is much less of a problem. Looking a little more I only found 2 uses in the global scope:

Also, I think it wouldnt be bad to remove this one as well: https://github.com/rapidsai/cuml/blob/branch-0.15/cpp/src_prims/matrix/matrix.cuh#L32

@teju85
Copy link
Member

teju85 commented Aug 1, 2020

My personal preference is to NOT allow this anywhere inside header files, certainly not in global scope! But, an exception can be made for inside:

  1. an anonymous namespace
  2. a function
  3. a class

I'd like to hear thoughts from @dantegd and @cjnolet as well here.

@github-actions
Copy link

This issue has been marked stale due to no recent activity in the past 30d. Please close this issue if no further response or action is needed. Otherwise, please respond with a comment indicating any updates or changes to the original issue and/or confirm this issue still needs to be addressed. This issue will be marked rotten if there is no activity in the next 60d.

@github-actions
Copy link

This issue has been marked rotten due to no recent activity in the past 90d. Please close this issue if no further response or action is needed. Otherwise, please respond with a comment indicating any updates or changes to the original issue and/or confirm this issue still needs to be addressed.

@mdemoret-nv
Copy link
Contributor Author

mdemoret-nv commented Feb 18, 2021

This issue was partially addressed by PR #3402. A quick search shows this issue is still needed but the impact is significantly reduced.

@github-actions
Copy link

This issue has been labeled inactive-90d due to no recent activity in the past 90 days. Please close this issue if no further response or action is needed. Otherwise, please respond with a comment indicating any updates or changes to the original issue and/or confirm this issue still needs to be addressed.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
? - Needs Triage Need team to review and classify bug Something isn't working CUDA / C++ CUDA issue inactive-90d Tech Debt Issues related to debt
Projects
None yet
Development

No branches or pull requests

4 participants