[libc-commits] [PATCH] D136288: [libc] add scanf parser and core utilities

[Siva Chandra via Phabricator via libc-commits]

sivachandra added inline comments.


================
Comment at: libc/src/stdio/scanf_core/core_structs.h:23
+// is why they are formatted differently from the rest of the file.
+enum class LengthModifier { hh, h, l, ll, j, z, t, L, none };
+
----------------
Bikeshed: Since the case is already mixed, why not just call it `NONE` instead of `none`?


================
Comment at: libc/src/stdio/scanf_core/core_structs.h:32
+struct ScanSet {
+  uint64_t bit_array[4] = {0, 0, 0, 0};
+  static constexpr size_t BITS_IN_UINT64 = 64;
----------------
We have a bitset class: https://github.com/llvm/llvm-project/blob/main/libc/src/__support/CPP/bitset.h


================
Comment at: libc/src/stdio/scanf_core/core_structs.h:45
+  // it is assumed that start < end.
+  void add_range(char start, char end) {
+    unsigned char ustart = static_cast<unsigned char>(start);
----------------
Add doc-strings explaining what this method is doing and its intended use case.


================
Comment at: libc/src/stdio/scanf_core/core_structs.h:80
+    for (size_t i = 0; i < sizeof(bit_array) / sizeof(uint64_t); ++i) {
+      bit_array[i] = ~bit_array[i];
+    }
----------------
Feel free to add a `flip` method to `cpp::bitset` in a separate CL - https://en.cppreference.com/w/cpp/utility/bitset/flip


================
Comment at: libc/src/stdio/scanf_core/core_structs.h:100
+  // Format Specifier Values
+  FormatFlags flags = FormatFlags(0);
+  LengthModifier length_modifier = LengthModifier::none;
----------------
Instead of literal `0`, use a symbolic flag value, say `NONE`. So, this then becomes:

```
FormatFlags flags = FormatFlags::NONE;
```


================
Comment at: libc/src/stdio/scanf_core/core_structs.h:142
+// This is the value to be returned by conversions when no error has occurred.
+constexpr int WRITE_OK = 0;
+// These are the scanf return values for when an error has occurred. They are
----------------
Why are they not enum constants?


================
Comment at: libc/src/stdio/scanf_core/parser.h:28
+#ifndef LLVM_LIBC_SCANF_DISABLE_INDEX_MODE
+  // args_start stores the start of the va_args, which is allows getting the
+  // value of arguments that have already been passed. args_index is tracked so
----------------
May be there is a typo somewhere in this comment?


================
Comment at: libc/src/stdio/scanf_core/scanf_config.h:12
+
+// #define LLVM_LIBC_SCANF_DISABLE_FLOAT
+// #define LLVM_LIBC_SCANF_DISABLE_INDEX_MODE
----------------
Add commentary explaining how are these to be used/tuned/configured.


Repository:
  rG LLVM Github Monorepo

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

https://reviews.llvm.org/D136288