Skip to content

Conversation

@sleeepyjack
Copy link
Collaborator

This PR adds some convenience macros for defining strong types.
Background: #429 (comment)

tl;dr We want to avoid defining strong types as type aliases of cuco::detail::strong_type as this leads to a situation where all strong types with the same underlying value type are indistinguishable:

using foo = cuco::detail::strong_type<float>;
using bar = cuco::detail::strong_type<float>;

void expects_foo(foo f) {
  ...
}

int main() {
  expects_foo(foo{0.1});
  expects_foo(bar{0.2}); // doesn't throw an error since foo and bar are aliases of the same type
}

@sleeepyjack sleeepyjack added type: bug Something isn't working Needs Review Awaiting reviews before merging labels Apr 2, 2024
@sleeepyjack sleeepyjack self-assigned this Apr 2, 2024
@sleeepyjack sleeepyjack requested a review from PointKernel as a code owner April 2, 2024 13:33
@PointKernel
Copy link
Member

I was thinking of something like this: https://godbolt.org/z/9Pzv71njW. All we need is to add a second template parameter with default and no need to update existing implementations.

@sleeepyjack
Copy link
Collaborator Author

All we need is to add a second template parameter with default and no need to update existing implementations.

I tried a similar thing earlier today but I found a few problems with this approach.
We basically never want std::is_same_v<A, C> to be true. Using the default tparam is a pitfall.
We need to specify a distinct phantom type each time we define a new strong type.

We actually don't need the tparam if we avoid declaring strong types through using aliases altogether, but instead define a new type which inherits from strong_type. That's what these macros are for.

@PointKernel
Copy link
Member

We basically never want std::is_same_v<A, C> to be true. Using the default tparam is a pitfall.

Valid point.

We actually don't need the tparam if we avoid declaring strong types through using aliases altogether, but instead define a new type which inherits from strong_type. That's what these macros are for.

I see. I guess me just not a macro fan then :), e.g.

  • scope: it's always in the global namespace
  • readability: exposing an empty_key<T> strong type via CUCO_DEFINE_TEMPLATE_STRONG_TYPE(empty_key) requires some efforts to understand/use

Copy link
Member

@PointKernel PointKernel left a comment

Choose a reason for hiding this comment

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

The goal of this PR is to resolve the strong type conflicts and the detail (like using macro or something else) can be adjusted upon use.

Looks great.

@sleeepyjack
Copy link
Collaborator Author

I see. I guess me just not a macro fan then :), e.g.
scope: it's always in the global namespace
readability: exposing an empty_key strong type via CUCO_DEFINE_TEMPLATE_STRONG_TYPE(empty_key) requires some efforts to understand/use

I'm also not a macro fan TBH but I can't find a better way to avoid typing the same code pattern for the struct definition over and over. 🙃

I added a few hints to the sentinel inline docs in my most recent commit so users at least know the names of the types defined by the macro.

@sleeepyjack sleeepyjack merged commit 0ae4c0e into NVIDIA:dev Apr 3, 2024
@sleeepyjack sleeepyjack deleted the fix-strong-type branch April 3, 2024 00:09
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

Needs Review Awaiting reviews before merging type: bug Something isn't working

Projects

None yet

Development

Successfully merging this pull request may close these issues.

2 participants