[libc-commits] [PATCH] D127985: [libc] add printf oct conversion

[Siva Chandra via Phabricator via libc-commits]

sivachandra added inline comments.


================
Comment at: libc/src/stdio/printf_core/oct_converter.h:32-33
+
+  // buff_cur can never reach 0, since the buffer is sized to always be able to
+  // contain the whole integer. This means that bounds checking it should be
+  // unnecessary.
----------------
Nitty nit: ISTM like this sentence is written in the oppositte way. You probably want:

```
Since the buffer is size to sized to be able fit the entire number, buf_cur can never reach 0.
So, we do not need bounds checking on buf_cur.
```


================
Comment at: libc/src/stdio/printf_core/oct_converter.h:39
+
+  size_t digits_written = BUFF_LEN - buff_cur;
+
----------------
Nit: The name `digits_written` can be confusing in the presence of a writer. You probably want to name it `digits_in_val`, or just `num_digits`.


================
Comment at: libc/src/stdio/printf_core/oct_converter.h:41
+
+  // these are signed to prevent underflow due to negative values. Negative
+  // values are treated the same as 0.
----------------
Nit: Please follow the convention of starting comments with upper case first letters. Here and elsewhere.


================
Comment at: libc/src/stdio/printf_core/oct_converter.h:92
+    if (zeroes > 0)
+      writer->write_chars('0', zeroes);
+    if (digits_written > 0)
----------------
Are writes guaranteed to succeed?


Repository:
  rG LLVM Github Monorepo

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

https://reviews.llvm.org/D127985