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

[Alex Brachet via Phabricator via libc-commits]

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


================
Comment at: libc/src/stdio/FILE.h:24
+  void *cookie;
+  cookie_write_function_t *write;
+};
----------------
sivachandra wrote:
> abrachet wrote:
> > MaskRay wrote:
> > > fopencookie is a use case. The type does not need to mention `cookie_*`.
> >  `cookie_write_function_t` is a type which must be exposed for `fopencookie`. That doesn't mean we need to refer to the internal type by that name though so I have just called it `write_function_t`
> I think having the `cookie` pointer is also a bit confusing. For example, it feels like you are saying all FILE objects should follow the cookie protocol (so to say). What do you think about this:
> 
> ```
> struct FILE {
>   lock
>   functions for file operations on FILE* objects
> };
> ```
> 
> For each specific kind of stream we will have a derived class of `FILE` which will contain stream specific data (For example, `cookie` in your case). Also, the stream creation functions like `fopen` will install the appropriate file operation functions in the FILE object. An operation function, say `seek`, will look like:
> 
> ```
> class MyFileType : public File {
>    ....
> };
> 
> int seek(FILE *f, long offset, ...) {
>   // This seek function is specific to MyFileType objects so the static down cast is safe.
>   MyFileType *myFile = static_cast<MyFileType *>(f):
>   ...
> }
> ```
> 
> As for `fopencookie`, I think you can wrap the `cookie_io_functions_t` functions in other functions which match the signature in the base `struct FILE` and avoid any type mismatch issues.
 This is what I was planning, so you're right cookie isn't necessary in the base FILE type. Also it makes more sense for the functions to take FILE I was thinking I would need to put that in the cookie which would have been a pain. Much better :)


================
Comment at: libc/src/stdio/FILE.h:17
+
+// TOOD: This should return ssize_t but it's defined in sys/types.h
+using write_function_t = size_t(void *, const char *, size_t);
----------------
sivachandra wrote:
> We probably want to avoid dependence on `sys/types.h` here to keep the definition of `FILE` platform independent. See my other comment below.
`ssize_t` is what `cookie_write_function_t` returns and this is defined in sys/types.h. The idea is that `write(2)` returns that I believe. I've removed that TODO for now.


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

https://reviews.llvm.org/D77626