Uint_pair feedback

[seanmiddleditch]

Overall looks pretty good, not overly complicated, looks like it does what it says on the tin with minimal fuss. :)

Much of the feedback below is subjective and opinion-based, so please feel very free to disagree with me on any of them without need for justification. :)

Naming

https://github.com/rilerez/bitpack/blob/762b55f770b78210c09da0df7e13e4ccb21bb130/src/include/bitpack/pair.hpp#L17

UInt_pair uses the names x and y throughout its implementation. Those names feel very appropriate for points or mathematical vectors, but for a type with "pair" in its name, I would expect the nomenclature to be more similar to std::pair, which uses first and second.

That said, given that the interface is not identical to std::pair, it could make sense to use different names entirely to avoid too many similarities. If you want to go that direct, I might also suggest high and low, or left and right, as more descriptive alternatives to x and y that won't innately imply mathematical-vector analogs.

Index lookup

https://github.com/rilerez/bitpack/blob/762b55f770b78210c09da0df7e13e4ccb21bb130/src/include/bitpack/pair.hpp#L55

I think you can make this fail via SFINAE instead of via a static_assert, which might result in better error messages; additionally it would allow users to use SFINAE to detect whether any given index is valid.

I think you'd need to change your nth machinery into using a struct and use specialization to define the types, e.g. something like:

  template <int> struct nth_helper;
  template <> struct nth_helper<0> { using type = X; };
  template <> struct nth_helper<1> { using type = Y; };
  template <int N> using nth = typename nth_helper<N>::type;

then nth<2> or whatnot would fail in a SFINAE-friendly way.

I'm not sure off-hand, but concepts might also provide an even better approach here than SFINAE, e.g. if you can write something like requires(N >= 0 && N <= 1).

Unitialized

https://github.com/rilerez/bitpack/blob/762b55f770b78210c09da0df7e13e4ccb21bb130/src/include/bitpack/pair.hpp#L26

The fields don't use NSDMI and the default constructor doesn't initialize them, so this type will be unitialized by default.

Since the Uint_pair type is effectively immutable (I don't see a way to modify the individual members) I'm not sure it's useful to have this type be unitialized by default, as there's fewer use cases where a developer would want an unitialized instance that they procedurally fill in later.

Operators

https://github.com/rilerez/bitpack/blob/762b55f770b78210c09da0df7e13e4ccb21bb130/src/include/bitpack/pair.hpp#L109

The reliance on conversion to std::pair feels especially unoptimal to me for a type like this. I'd somewhat expect you'd be able to implement operator== easily in terms of the underlying x_ and y_ values. Same for operator<=> too, just a little less simple. :)

Macro leakage

https://github.com/rilerez/bitpack/blob/762b55f770b78210c09da0df7e13e4ccb21bb130/src/include/bitpack/pair.hpp#L98

For "local" macros like that, it is good practice to #undef the macros after you've used them in the header.

Non-hidden operators

https://github.com/rilerez/bitpack/blob/762b55f770b78210c09da0df7e13e4ccb21bb130/src/include/bitpack/pair.hpp#L107

Generally try to prefer using hidden friends for operators and extension points. That both helps compiler throughput by reducing the number of overloads it needs to consider when resolving operators for unrelated types, and improves error messages for users when they misuse operators for unrelated types (as many compilers list out all the possible overloads it found).

"hidden friends" just mean that you define the friend function inside the class body, which makes the function only discoverable via ADL. That way they're only in the overload set when one of the parameters matches the type in question.

Consider reducing "nesting" of function calls

https://github.com/rilerez/bitpack/blob/762b55f770b78210c09da0df7e13e4ccb21bb130/src/include/bitpack/pair.hpp#L46

In a few places in the implementation, functions will defer to other functions for implementation. While this is good engineering practice to reduce code duplication, it comes at a cost. Namely, in a non-optimized build, every function call has invocation overhead which adds up for small accessors; for optimized builds, every function call is more work for the optimizer to chew through. Likewise for constexpr invocation, more indirection means the compiler has more work to do on evaluation.

A guideline I tend to follow: if a function is a single return statement, duplicate it instead of forwarding to it. It's duplicated code, but it's small and simple and easy to maintain, and the resulting improvements in debug-codegen or optimizer-time are (scaled up across a codebase, if types follow the guidelines everywhere) very noticeable.

Missing constexpr and noexcept

https://github.com/rilerez/bitpack/blob/762b55f770b78210c09da0df7e13e4ccb21bb130/src/include/bitpack/pair.hpp#L107

The comparison operators should be noexcept given the types involved, I'd imagine. Likewise, they should be able to be constexpr.

[rileylev]

I don't know why I just saw this now but thank you! I'll read it over