[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);
----------------
michaelrj wrote:
> 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`.
> 
> 
No, that is not an overkill - you are keeping all file operations cohesively managed by the `FileWriter`.

+1 for the `vfprintf_internal` refactoring.


================
Comment at: libc/src/stdio/printf_core/writer.h:41
+  // negative value.
   void write(const char *new_string, size_t length);
 
----------------
michaelrj wrote:
> 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.
Normal design practice would be to return as soon as you see an error. We should follow that until we have sufficient evidence that conditionals like that are the source of a performance problem. Even if we can conclude that conditionals like that are a performance problem, a potential solution should investigate other kind of refactorings which reduce conditionals while stopping as soon as an error is detected.


Repository:
  rG LLVM Github Monorepo

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

https://reviews.llvm.org/D126831