[libcxx-commits] [libcxx] [libc++] Improve bitset::to_ullong Implementation (PR #121348)

[Peng Liu via libcxx-commits]

================
@@ -381,8 +382,9 @@ __bitset<_N_words, _Size>::to_ullong(true_type, true_type) const {
   unsigned long long __r = __first_[0];
   _LIBCPP_DIAGNOSTIC_PUSH
   _LIBCPP_GCC_DIAGNOSTIC_IGNORED("-Wshift-count-overflow")
-  for (size_t __i = 1; __i < sizeof(unsigned long long) / sizeof(__storage_type); ++__i)
-    __r |= static_cast<unsigned long long>(__first_[__i]) << (sizeof(__storage_type) * CHAR_BIT);
+  size_t __n_words = std::min<size_t>(_N_words, sizeof(unsigned long long) / sizeof(__storage_type));
----------------
winner245 wrote:

Thank you for your feedback! I acknowledge that I didn't provide an accurate description of the changes. Your suggested title looks good to me, and I will update it accordingly.

I agree that adding full support for 16-bit systems would require more work, and I haven't discussed that aspect yet. I initiated this PR because I noticed that the following loop in the current implementation appears incorrect for `__i > 1`:

```
for (size_t __i = 1; __i < sizeof(unsigned long long) / sizeof(__storage_type); ++__i)
  __r |= static_cast<unsigned long long>(__first_[__i]) << (sizeof(__storage_type) * CHAR_BIT);
```

In this loop, each word is shifted by the same amount, `__bits_per_word` (which equals `sizeof(__storage_type) * CHAR_BIT`), and this is only correct for `__i == 1`. For `__i >= 2`, the`i`-th word needs to be shifted by `i * __bits_per_word` to concatenate correctly.

This observation led me to consider scenarios where `sizeof(unsigned long long) / sizeof(__storage_type) > 2`, resulting in multiple iterations of the loop, which would cause it to be incorrect. The 16-bit systems came to mind as one example.

If our goal is to fully support 16-bit systems, I believe we need a more thorough investigation of other operations. Specifically, we should revisit the constructor that takes unsigned long long, as it currently only considers `sizeof(size_t) = 4` and `8`, which may only account for 32-bit and 64-bit systems. I would greatly appreciate any suggestions or ideas you may have.

https://github.com/llvm/llvm-project/pull/121348