Repository: carbon-language/carbon-lang
File: common/raw_hashtable_metadata_group.h (C/C++)

Code before:
    auto operator==(const Iterator& rhs) const -> bool {
      return bits_ == rhs.bits_;
    }

    auto operator*() -> ssize_t& {
      CARBON_DCHECK(bits_ != 0, "Cannot get an index from zero bits!");
      __builtin_assume(bits_ != 0);
      index_ = BitIndexT(bits_).index();
      // Note that we store the index in a member so we can return a reference
      // to it here as required to be a forward iterator.
      return index_;
    }

    template <typename T>
    auto index_ptr(T* pointer) -> T* {
      return BitIndexT(bits_).index_ptr(pointer);
    }

    auto operator++() -> Iterator& {
      CARBON_DCHECK(bits_ != 0, "Must not increment past the end!");
      __builtin_assume(bits_ != 0);

      if constexpr (ByteEncodingMask != 0) {
        // Apply an increment mask to the bits first. This is used with the byte
        // encoding when the mask isn't needed until we begin incrementing.
        static_assert(BitIndexT::ByteEncoding);
        bits_ &= ByteEncodingMask;
      }

      // Clears the least significant set bit, effectively stepping to the next
      // match.
      bits_ &= (bits_ - 1);
      return *this;
    }

   private:
    ssize_t index_;
    BitsT bits_ = 0;
  };

  BitIndexRange() = default;
  explicit BitIndexRange(BitsT bits) : bits_(bits) {}

  explicit operator bool() const { return !empty(); }
  auto empty() const -> bool { return BitIndexT(bits_).empty(); }

  auto begin() const -> Iterator { return Iterator(bits_); }
  auto end() const -> Iterator { return Iterator(); }

  friend auto operator==(BitIndexRange lhs, BitIndexRange rhs) -> bool {
    if constexpr (ByteEncodingMask == 0) {

Review comment (bug):
I'm looking at this because it's the only use of `BitIndexT::ByteEncoding` (the surrounding code doesn't access it outside the `static_assert`). I believe that because this is inside `if constexpr` removing it is not a compile failure on most platforms (i.e., I'm on x86, I can freely revert the `static constexpr bool` addition without a compile error).

Had you considered shifting this to make it a compile error, like a class-level `static_assert(ByteEncodingMask == 0 || BitIndexT::ByteEncoding);` or maybe something with `requires`?

Code after:
    explicit Iterator(BitsT bits) : bits_(bits) {}

    auto operator==(const Iterator& rhs) const -> bool {
      return bits_ == rhs.bits_;
    }

    auto operator*() -> ssize_t& {
      CARBON_DCHECK(bits_ != 0, "Cannot get an index from zero bits!");
      __builtin_assume(bits_ != 0);
      index_ = BitIndexT(bits_).index();
      // Note that we store the index in a member so we can return a reference
      // to it here as required to be a forward iterator.
      return index_;
    }

    template <typename T>
    auto index_ptr(T* pointer) -> T* {
      return BitIndexT(bits_).index_ptr(pointer);
    }

    auto operator++() -> Iterator& {
      CARBON_DCHECK(bits_ != 0, "Must not increment past the end!");
      __builtin_assume(bits_ != 0);

      if constexpr (ByteEncodingMask != 0) {
        // Apply an increment mask to the bits first. This is used with the byte
        // encoding when the mask isn't needed until we begin incrementing.
        bits_ &= ByteEncodingMask;
      }

      // Clears the least significant set bit, effectively stepping to the next
      // match.
      bits_ &= (bits_ - 1);
      return *this;
    }

   private:
    ssize_t index_;
    BitsT bits_ = 0;
  };

  BitIndexRange() = default;
  explicit BitIndexRange(BitsT bits) : bits_(bits) {}

  explicit operator bool() const { return !empty(); }
  auto empty() const -> bool { return BitIndexT(bits_).empty(); }

  auto begin() const -> Iterator { return Iterator(bits_); }
  auto end() const -> Iterator { return Iterator(); }

  friend auto operator==(BitIndexRange lhs, BitIndexRange rhs) -> bool {