rapidsai / cuml

cuML - RAPIDS Machine Learning Library
https://docs.rapids.ai/api/cuml/stable/
Apache License 2.0
4.2k stars 525 forks source link

[BUG] Remove `using namespace` from header files #2630

Open mdemoret-nv opened 4 years ago

mdemoret-nv commented 4 years ago

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.

drobison00 commented 4 years ago

@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 commented 4 years ago

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 commented 4 years ago

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[bot] commented 3 years ago

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[bot] commented 3 years ago

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 commented 3 years ago

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[bot] commented 3 years ago

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.