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

[Siva Chandra via Phabricator via libc-commits]

sivachandra added inline comments.


================
Comment at: libc/test/src/string/strspn_test.cpp:13
+
+TEST(StrSpnTest, EmptyStringShouldReturnZero) {
+  // The search should not include the null terminator.
----------------
Seems to me like all of these tests should just be combined into one test as the different names are making it more confusing.


================
Comment at: libc/test/utils/CPP/bitset_test.cpp:1
+//===-- Unittests for bitset ----------------------------------------------===//
+//
----------------
Uppercase `*B*itset`.


================
Comment at: libc/test/utils/CPP/bitset_test.cpp:57
+
+TEST(BitsetTest, SetsProperBit) {
+  for (size_t i = 0; i < 256; ++i) {
----------------
Seems like `SetsProperBit` and `DoesNotSetNeighbotBts` should be combined into something like `SettingXDoesNotSetY`.


================
Comment at: libc/test/utils/CPP/bitset_test.cpp:79
+      if (neighbor == i)
+        continue;
+      EXPECT_FALSE(bitset.test(neighbor));
----------------
May be `EXPECT_TRUE(...)`.


================
Comment at: libc/test/utils/CPP/bitset_test.cpp:85
+
+TEST(BitsetTest, SettingBitXShouldNotResetBitY) {
+  __llvm_libc::cpp::Bitset<128> bitset;
----------------
s/`Should`/`Does` ?


================
Comment at: libc/utils/CPP/Bitset.h:18
+
+constexpr size_t CharacterBit = 8;
+constexpr size_t BitsPerUnit = CharacterBit * sizeof(uintptr_t);
----------------
s/`CharacterBit`/`BitsPerByte` ?


================
Comment at: libc/utils/CPP/Bitset.h:22
+// Calculates the array size for the bitset. Adds 'CharacterBit - 1' in length
+// to avoid rounding down. If calculated size == 0, then returns 1. Otherwise,
+// returns the appropriate size.
----------------
If `number_of_bits` is always > zero, will the calculated size be zero?


================
Comment at: libc/utils/CPP/Bitset.h:43
+private:
+  uintptr_t Data[GetArraySize(NumberOfBits)] = {0};
+};
----------------
Because there is a static assert to ensure `NumberOfBits` is not 0, why not just do:

```
uintptr_t Data[(NumberOfBits + BitsPerByte - 1)/BitsPerUnit] = {0};
```


Repository:
  rG LLVM Github Monorepo

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

https://reviews.llvm.org/D85103