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

[Siva Chandra via Phabricator via libc-commits]

sivachandra added inline comments.


================
Comment at: libc/src/stdio/FILE.h:24
+  void *cookie;
+  cookie_write_function_t *write;
+};
----------------
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.


================
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);
----------------
We probably want to avoid dependence on `sys/types.h` here to keep the definition of `FILE` platform independent. See my other comment below.


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

https://reviews.llvm.org/D77626