[libc-commits] [PATCH] D151282: [libc] Add initial support for 'puts' and 'fputs' to the GPU

[Siva Chandra via Phabricator via libc-commits]

sivachandra added inline comments.


================
Comment at: libc/src/__support/File/file.cpp:35
+    return write_unlocked_fbf(static_cast<const uint8_t *>(data), len);
+  } else /*if (bufmode == _IOLBF) */ { // unbuffered
+    return write_unlocked_lbf(static_cast<const uint8_t *>(data), len);
----------------
`line buffered`


================
Comment at: libc/src/__support/File/file.h:186
 
+  // We do not want to emit a destructor for the GPU build.
+#if defined(LIBC_TARGET_ARCH_IS_GPU)
----------------
Can you add a comment why?


================
Comment at: libc/src/__support/File/file.h:43
+// out the buffering portions of the code in the general implementation.
+#if defined(LIBC_TARGET_ARCH_IS_GPU)
+  static constexpr bool ENABLE_BUFFER = false;
----------------
jhuber6 wrote:
> sivachandra wrote:
> > Sorry, forgot to mention this in the earlier post: Just pointing out that files opened even with `fopencookie` will become unbuffered.
> > 
> > That we have new switch means we should add tests for it on non-GPU platforms also. So, one way would be to do something like this:
> > 1. Add a compile option, say `LIBC_COPT_UNBUFFERED_FILE` and update this conditional to:
> > ```
> > #if defined(LIBC_TARGET_ARCH_IS_GPU) || defined(LIBC_COPT_UNBUFFERED_FILE)
> > ```
> > 2. The above flag will allow to setup up a separate platform independent test only target for unbuffered files.
> > 3. Add a test for unbuffered files which depends on the ubuffered file target.
> It would be fine to have other users of unbuffered IO, maybe for a more minimal implementation.
I agree that it is fine to have this switch. My comment about `fopencookie` was just FYI. But, to protect accidental regressions around this switch, I am suggesting that you should add a test for it even for non-GPU platforms. 


================
Comment at: libc/src/__support/File/gpu/dir.cpp:16
+ErrorOr<int> platform_opendir(const char *name) {
+  return __llvm_libc::Error(-1);
+}
----------------
jhuber6 wrote:
> sivachandra wrote:
> > If they are just erroring out, why are they required?
> Wasn't sure if this was required for linking somewhere.
You are seeing some linker error if you don't add this file?


Repository:
  rG LLVM Github Monorepo

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

https://reviews.llvm.org/D151282