[libc-commits] [PATCH] D85103: [libc] Add strspn implementation.

[Chris Gyurgyik via Phabricator via libc-commits]

cgyurgyik marked 3 inline comments as done.
cgyurgyik added inline comments.


================
Comment at: libc/test/src/string/strspn_test.cpp:13
+
+TEST(StrSpnTest, EmptyStringShouldReturnZero) {
+  // The search should not include the null terminator.
----------------
sivachandra wrote:
> Seems to me like all of these tests should just be combined into one test as the different names are making it more confusing.
Can you clarify what you mean here?
I'm trying to unit test `strspn`, i.e. test different behaviors so that if it fails for behavior X, then behavior X unit test should fail rather than all of them. This has helped me already with some small debugging problems when manipulating bits. 

I agree that my naming isn't phenomenal, but its hard when trying to talk about spans and segments. I an open to naming changes and combining certain tests, but I don't know if combining all the unit tests is the correct approach.

With that said, I've made some changes to the names in hopes to improve this.



================
Comment at: libc/utils/CPP/Bitset.h:43
+private:
+  uintptr_t Data[GetArraySize(NumberOfBits)] = {0};
+};
----------------
sivachandra wrote:
> Because there is a static assert to ensure `NumberOfBits` is not 0, why not just do:
> 
> ```
> uintptr_t Data[(NumberOfBits + BitsPerByte - 1)/BitsPerUnit] = {0};
> ```
Let
```
NumberOfBits = 16
BitsPerByte = 8
BitsPerUnit = BitsPerByte * sizeof(uintptr_t) = 8 * 4
```

```
(NumberOfBits + BitsPerByte - 1) / BitsPerUnit
= (16 + 8 - 1) / (8 * 4)
= 23 / 32
= 0
```
If `(NumberOfBits + BitsPerByte - 1) < BitsPerUnit`, then we get 0.
I can simplify this further and just say if NumberOfBits < BitsPerUnit, return 1 unit.


Repository:
  rG LLVM Github Monorepo

CHANGES SINCE LAST ACTION
  https://reviews.llvm.org/D85103/new/

https://reviews.llvm.org/D85103