[ESIMD] Fix leakage of detail namespace

[DenisBakhvalov]

This patch removes using namespace sycl::INTEL::gpu::detail; from esimd.hpp header file, which leaks names into the global namespace.

[kbobrovs]

This patch removes using namespace sycl::INTEL::gpu::detail; from esimd.hpp header file, which leaks names into the global namespace.

I'm not sure what problem this patch solves. Please elaborate

[DenisBakhvalov]

This patch removes using namespace sycl::INTEL::gpu::detail; from esimd.hpp header file, which leaks names into the global namespace.

I'm not sure what problem this patch solves. Please elaborate

The expression using namespace declared in the header file essentially makes everything in that namespace available in the global namespace. This is wrong since users can accidentally use some APIs from that hidden namespace.

[AaronBallman]

LGTM!

One question I have is: did you search to see if there are other using namespace directives as well?

[kbobrovs]

The expression using namespace declared in the header file essentially makes everything in that namespace available in the global namespace. This is wrong since users can accidentally use some APIs from that hidden namespace.

I don't think this is the case. using namespace sycl::INTEL::gpu::detail; is within sycl::INTEL::gpu namespace, so unqualified names from sycl::INTEL::gpu::detail are not made available in the global namespace by inclusion of esimd.hpp. They are made available only within sycl::INTEL::gpu.
This example below won't compile, which means no leak into global namespace happens:

namespace a {
  class A{};
}

namespace b {
  using namespace a;
  class B : public A{};
}

A f_in_global_namespace() {
  return A{};
}

[kbobrovs]

I can see the problem now. For symbols to be leaked it is not enough to have this removed using, user code needs to have using sycl::INTEL::gpu too. The example below will compile, which models the actual problem:

namespace a {
  class A{};
}

namespace b {
  using namespace a;
  class B : public A{};
}

using namespace b; // <----- second required component of the leak.

A f_in_global_namespace() {
  return A{};
}

[kbobrovs]

One question I have is: did you search to see if there are other using namespace directives as well?

I saw these:

$ grep -R 'using namespace' sycl/include
sycl/include/CL/sycl/ONEAPI/intel_matrix/matrix-amx.hpp:using namespace cl::sycl;
sycl/include/CL/sycl/ONEAPI/intel_matrix/matrix-amx.hpp:using namespace cl::sycl::ONEAPI;
sycl/include/CL/sycl/ONEAPI/atomic_ref.hpp:using namespace ::cl::sycl::detail;
sycl/include/CL/sycl/ONEAPI/atomic_fence.hpp:using namespace cl::sycl::detail;
sycl/include/CL/sycl/INTEL/esimd/detail/esimd_types.hpp:using namespace sycl::INTEL::gpu;
$

The last three do it from within ...::detail namespace, so I assume this should not present problems (?), as users are not supposed to have using namespace ...::detail;.
The first two seem problematic - will (unintendedly) leak unqualified public APIs names when legitimate using namespace sycl::ext::intel::matrix; is present in the global ns.

+ @romanovvlad

[DenisBakhvalov]

I can see the problem now. For symbols to be leaked it is not enough to have this removed using, user code needs to have using sycl::INTEL::gpu too. The example below will compile, which models the actual problem:

namespace a {
  class A{};
}

namespace b {
  using namespace a;
  class B : public A{};
}

using namespace b; // <----- second required component of the leak.

A f_in_global_namespace() {
  return A{};
}

Yes, I think it is a general rule that using namespace should never be used in header files.

[AaronBallman]

I can see the problem now. For symbols to be leaked it is not enough to have this removed using, user code needs to have using sycl::INTEL::gpu too. The example below will compile, which models the actual problem:

namespace a {
  class A{};
}

namespace b {
  using namespace a;
  class B : public A{};
}

using namespace b; // <----- second required component of the leak.

A f_in_global_namespace() {
  return A{};
}

Yes, I think it is a general rule that using namespace should never be used in header files.

+1 (outside of situations in which it cannot leak, such as within a function body, as may be needed when considering ADL).