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

Fix namespace lookup issues in <oneapi/dpl/random> header #1855

Merged
merged 2 commits into from
Sep 19, 2024

Conversation

dmitriy-sobolev
Copy link
Contributor

@dmitriy-sobolev dmitriy-sobolev commented Sep 17, 2024

<oneapi/dpl/random> relies on namespace lookup mechanism which does not work well in some cases. There are compilation issues if <oneapi/dpl/dynamic_selection> is included first.

Simplified reproducer of the issue with the namespace lookup:

namespace dpl::experimental::internal{}

namespace dpl {
    namespace internal {
        template <typename T>
        void feature() {}
    }
    namespace experimental {
        template <typename U>
        void test() {
            internal::feature<U>();
        }
    }
}

>>  error: no template named 'feature' in namespace 'dpl::experimental::internal'; did you mean '::dpl::internal::feature'?

dynamic_selection adds oneapi::dpl::experimental::internal namespace, and a compiler stops looking for other namespaces after encountering it. Hence, it fails to find entities such as oneapi::dpl::internal::element_type_t.

Copy link
Contributor

@aelizaro aelizaro left a comment

Choose a reason for hiding this comment

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

Looks good to me, thank you for the fix!

Am I right, that we don't have to do the same thing in other <random> functionality, as it is only affected experimental namespace? https://github.com/oneapi-src/oneDPL/blob/6cd6678cfe51fff4767a8acab51ae1f2c4a4d3ed/include/oneapi/dpl/internal/random_impl/linear_congruential_engine.h#L28

@dmitriy-sobolev
Copy link
Contributor Author

Looks good to me, thank you for the fix!

Am I right, that we don't have to do the same thing in other <random> functionality, as it is only affected experimental namespace?

https://github.com/oneapi-src/oneDPL/blob/6cd6678cfe51fff4767a8acab51ae1f2c4a4d3ed/include/oneapi/dpl/internal/random_impl/linear_congruential_engine.h#L28

I still cannot grasp the namespace lookup rules completely as they are stated by the standard. However, I have not observed the issues here and I think it is expected. Here is my speculation. Firstly, the compiler looks for entities defined in its own namespace, secondly, in the nested namespaces, and lastly in the namespaces on the higher levels. The case with internal::element_type_t being searched from oneapi::dpl falls into the bin with "nested namespaces". The fixed issue happens when internal::element_type_t is being searched from oneapi::dpl::experimental, which falls into the bin with "namespaces on the higher levels", which is less reliable.

@dmitriy-sobolev dmitriy-sobolev merged commit e3a4288 into main Sep 19, 2024
23 checks passed
@dmitriy-sobolev dmitriy-sobolev deleted the dev/dmitriy-sobolev/fix-dpl-random branch September 19, 2024 09:39
mmichel11 pushed a commit that referenced this pull request Sep 25, 2024
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
Projects
None yet
Development

Successfully merging this pull request may close these issues.

2 participants