-
Notifications
You must be signed in to change notification settings - Fork 75
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
prevent allocMappedBufIfSupported
from accepting arbitrary Types as TPlatform
#2120
prevent allocMappedBufIfSupported
from accepting arbitrary Types as TPlatform
#2120
Conversation
@chaever Thanks for your first PR. We will have a look into it! |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Thank you!
include/alpaka/mem/buf/Traits.hpp
Outdated
@@ -175,9 +177,10 @@ namespace alpaka | |||
TPlatform const& platform, | |||
TExtent const& extent = TExtent()) | |||
{ | |||
if constexpr(hasMappedBufSupport<TPlatform>) | |||
using PlatformForMappedAlloc = Platform<TPlatform>; |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I would suggest
using PlatformForMappedAlloc = Platform<TPlatform>; | |
using Platform = alpaka::Platform<TPlatform>; |
if it works?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
See for example
using Platform = alpaka::Platform<Dev>; |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
This should now be consistent with the other usages of the Platform
-Trait, which all have the alpaka
namespace prefixed.
include/alpaka/mem/buf/Traits.hpp
Outdated
@@ -175,9 +177,10 @@ namespace alpaka | |||
TPlatform const& platform, | |||
TExtent const& extent = TExtent()) | |||
{ | |||
if constexpr(hasMappedBufSupport<TPlatform>) | |||
using PlatformForMappedAlloc = alpaka::Platform<TPlatform>; |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
can you use using Platform
like everywhere else?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
That was the initial version, until this was pointed out.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
My assumption was wrong. Platform = Platform<T>
doesn't trigger -Wshadow
. So feel free to follow @fwyzard's suggestion.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
PlatformForMappedAlloc
should now be replaced by Platform
.
@chaever Could you please call |
4156839
to
3709711
Compare
Can you do a |
…r a shadow warning
3709711
to
a0ca64a
Compare
This branch should now be branching off from develop at 6b43b8d (most recent develop-commit at time of writing). |
This PR aims to resolve #2065.