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

[Chris Gyurgyik via Phabricator via libc-commits]

cgyurgyik added inline comments.


================
Comment at: libc/src/string/strspn.cpp:17
+// 8 bits of a character, multiplied by a factor of sizeof(size_t).
+static constexpr size_t EXPLODED_CHAR_BIT = 8 * sizeof(size_t);
+
----------------
abrachet wrote:
> `static` not necessary. Also I think that the naming convention is wrong here, it should be the same as all other identifiers.
Yeah I couldn't find much on constants naming convention. Changed to lowercase.


================
Comment at: libc/src/string/strspn.cpp:19-20
+
+// The size of the byteset used. The following must hold:
+// 1 <= sizeof(size_t) <= 32.
+static constexpr size_t BYTESET_SIZE = 32 / sizeof(size_t);
----------------
abrachet wrote:
> `The size of the byteset used.` Not necessary.
> The rest if you think that is actually a concern, presumably not then we should `static_assert` it otherwise remove that comment too.
I don't know any systems that have a size_t > 32 bytes. I'll remove this for now.


================
Comment at: libc/src/string/strspn.cpp:24
+// Toggles the bit produced by 'c' in the byteset provided.
+static inline void toggle_bit(size_t *byteset, unsigned char c) {
+  const size_t ch = c;
----------------
abrachet wrote:
> `inline` not necessary. Instead of taking an `unsigned char` then assigning it to a `size_t` just take a `size_t` here and in `contains_bit`.
Is there any harm in `inline` here? I removed it, but for my own knowledge. From what I understand, you're saying the compiler will inline it on its own accord, and this is just extra unnecessary specifier?


================
Comment at: libc/src/string/strspn.cpp:31
+// Determines whether the byteset contains the bit produced by 'c'.
+static inline bool contains_bit(size_t *byteset, unsigned char c) {
+  const size_t ch = c;
----------------
abrachet wrote:
> `byteset` should be a `const size_t *`
Good catch thanks.


================
Comment at: libc/src/string/strspn.cpp:42-51
+  if (!segment[0])
+    return 0;
+
+  const char *initial = src;
+  if (!segment[1]) {
+    // The segment is a single character.
+    for (; *src == *segment; ++src)
----------------
abrachet wrote:
> The difference in speed here is going to be negligible, the question is wether you think it is worth complicating the code for these edge cases.
This is something Siva hit me on before as well, so I'll wait for his input here too. I understand your thinking, but I don't find it grossly complicated, and think its good to avoid creating a bitset unless absolutely necessary. 


Repository:
  rG LLVM Github Monorepo

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

https://reviews.llvm.org/D85103