[libc-commits] [PATCH] D126831: [libc] add printf

[Siva Chandra via Phabricator via libc-commits]

sivachandra added inline comments.


================
Comment at: libc/src/stdio/printf.cpp:33
 
+  __llvm_libc::stdout->lock();
   int ret_val = printf_core::printf_main(&writer, format, args);
----------------
Nit: There is an RAII file lock:

```
__llvm_libc::File::FileLock lock(__llvm_libc::stdout);
```

Couple of things more:
1. It seems to me like this is not the place to do the lock/unlock and call to error_unlocked. It should be managed by the file writer and the errors should be surfaced to printf_main. See another comment below.
2. `printf` and `fprintf` should share code in some manner instead of being mostly duplicated.


================
Comment at: libc/src/stdio/printf_core/writer.h:41
+  // negative value.
   void write(const char *new_string, size_t length);
 
----------------
If this method returns an int value, which can be negative on error, printf_main can do an early return as soon as it sees an error. If this is OK, you should do the same with the converter functions. Why is that not a preferred design?


Repository:
  rG LLVM Github Monorepo

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

https://reviews.llvm.org/D126831