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

[Alex Brachet via Phabricator via libc-commits]

abrachet added a comment.

FWIW, I think in most real world scenarios the O(N*M) approach would be faster. Granted I don't know what those scenarios are I had never heard of this function, I have no idea where this is useful.



================
Comment at: libc/src/string/strspn.cpp:16
+
+// 8 bits of a character, multiplied by a factor of sizeof(size_t).
+static constexpr size_t EXPLODED_CHAR_BIT = 8 * sizeof(size_t);
----------------
None of the comments in this file are necessary I would say. This code explains itself.


================
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);
+
----------------
`static` not necessary. Also I think that the naming convention is wrong here, it should be the same as all other identifiers.


================
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);
----------------
`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.


================
Comment at: libc/src/string/strspn.cpp:21
+// 1 <= sizeof(size_t) <= 32.
+static constexpr size_t BYTESET_SIZE = 32 / sizeof(size_t);
+
----------------
32 is supposed to be how many bytes it takes to represent 256 bits? Maybe `(256 / 8) / sizeof(size_t)` is easier to read? This might have actually warranted a comment

Also should this be called a bitset not a byteset?


================
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;
----------------
`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`.


================
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;
----------------
`byteset` should be a `const size_t *`


================
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)
----------------
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.


Repository:
  rG LLVM Github Monorepo

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

https://reviews.llvm.org/D85103