[libc-commits] [PATCH] D122773: [libc][NFC] add outline of printf
Michael Jones via Phabricator via libc-commits
libc-commits at lists.llvm.org
Thu Mar 31 14:47:48 PDT 2022
michaelrj added inline comments.
================
Comment at: libc/src/stdio/printf_files/parser.h:18
+namespace __llvm_libc {
+namespace internal {
+namespace printf_core {
----------------
sivachandra wrote:
> Do we really need an `internal` namespace here? You normally introduce an internal namespace to indicate that the interface declared in such a namespace is not to be used by clients of the higher level namespace. If we do need something internal, ISTM that the nesting should be reversed? As in, `internal` should be nested inside `printf_core`?
I've removed the `internal` namespace and moved everything to `printf_core`.
================
Comment at: libc/src/stdio/printf_files/writer.h:20
+
+class Writer {
+ void *output;
----------------
sivachandra wrote:
> Is this class envisioned to be a `final` class? If yes, then we should have documentation of the expectations. For example, what is `output`, `raw_write` etc. If not also, we should add documentation about how it is to be specialized.
I was under the impression that virtual functions were disallowed in libc, so all classes were implicitly final.
I've added comments with more explanation on how this specialization works.
================
Comment at: libc/src/stdio/printf_files/writer.h:33
+
+ size_t get_chars_written();
+};
----------------
sivachandra wrote:
> Why is size required at all?
the number of chars written is the return value for printf, as well as the value written by %n.
Repository:
rG LLVM Github Monorepo
CHANGES SINCE LAST ACTION
https://reviews.llvm.org/D122773/new/
https://reviews.llvm.org/D122773
More information about the libc-commits
mailing list