[libc-commits] [PATCH] D77626: [libc] Add very basic stdio FILE and fwrite

[Alex Brachet via Phabricator via libc-commits]

abrachet marked 5 inline comments as done.
abrachet added inline comments.


================
Comment at: libc/src/stdio/FILE.h:20
+
+struct FILE {
+  mtx_t lock;
----------------
sivachandra wrote:
> May be this struct is scalable. As in, this structure can handle all kinds of FILE objects. But, few questions below.
Yes I think we could have internal types which inherit from this base `__llvm_libc::FILE`.


================
Comment at: libc/src/stdio/FILE.h:23
+
+  void *cookie;
+  cookie_write_function_t *write;
----------------
sivachandra wrote:
> For a simple use like this, a name like `cookie` seems alright. But, would it make sense to call it `write_ptr`?
I think the name isn't great either but its from [[ http://man7.org/linux/man-pages/man3/fopencookie.3.html | `fopencookie` ]] which is a GNU extension, I think is worthwhile to support. Wether we do ever export `fopencookie` or not it is a good internal scheme.


================
Comment at: libc/src/stdio/FILE.h:24
+  void *cookie;
+  cookie_write_function_t *write;
+};
----------------
sivachandra wrote:
> Should we actually specify the type of the writing function? Let it be set by the FILE creation function to the appropriate writer type?
What do you mean? Templatize it for internal use?


================
Comment at: libc/src/stdio/fwrite.h:17
+
+size_t fwrite_unlocked(const void *__restrict ptr, size_t size, size_t nmeb,
+                       FILE *__restrict stream);
----------------
sivachandra wrote:
> Asking for my own knowledge: when/where would an internal unlock version be useful?
Possibly in functions like `printf` which might have separated writes but the stream would need to remain locked to make sure it is written continuously. This may not be ever called internally though. Should I remove its prototype from the internal header?


================
Comment at: libc/test/config/linux/x86_64/syscall_test.cpp:12
 
-#include <functional>
+namespace __llvm_libc {
+
----------------
sivachandra wrote:
> Does it make sense to move it utils/CPP? It is incomplete right now, but may be utils/CPP is a more appropriate place.
Possibly, I just have this here for the type checking. All it really needs is an `operator()` and it would be pretty similar to `std::function`, so we could put it there.

Also, while we are talking about it. Would utils/CPP be an appropriate place to add an analog to `std::unique_lock` for internal use around `mtx_t`?


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

https://reviews.llvm.org/D77626