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

[Michael Jones via Phabricator via libc-commits]

michaelrj 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);
----------------
sivachandra wrote:
> 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.
For the file lock class, that isn't a public class so I can't access it here.

I've moved some error handling to printf_main.
As for managing the long in the file writer, the problem is that there is no file writer. Since the FILE class already exposes the interface needed, the write_to_file function just uses the raw FILE. While I could create a class just to hold a lock and destroy it on exit, that seems a bit overkill when I can just call the lock and unlock functions by hand.

I've moved the shared `printf` and `fprintf` implementation into `vfprintf_internal`.




================
Comment at: libc/src/stdio/printf_core/writer.h:41
+  // negative value.
   void write(const char *new_string, size_t length);
 
----------------
sivachandra wrote:
> 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?
It's not a preferred design because `write` may be called multiple times in a single function. Int conversion uses up to 4 calls. Having an `if result < 0` after each of those would be very inefficient, and more importantly not particularly helpful. Usually if a write fails, then all subsequent writes will also fail, so I think it's best to not check every write, instead check the status of the writer every loop. I've added a condition in `printf_main` to do just that.


Repository:
  rG LLVM Github Monorepo

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

https://reviews.llvm.org/D126831