[PATCH] D67867: [libc] Add few docs and implementation of strcpy and strcat.

[James Y Knight via Phabricator via llvm-commits]

jyknight added a comment.

Some high level comments on your filesystem layout standard:

I think one directory per function is going to be annoying, not helpful. Most libc functions are expressible as a single function of less than 100 LOC. I'd suggest, instead, to have e.g. `src/stdlib/strcpy.cc`, implementing the strcpy function. It seems like a good rule that every file implements one public function. If additional files are needed for implementation clarity, I'd suggest to put such helper code into something like `src/stdlib/internal/strcpy_internal.cpp` (obviously not _actually_ for strcpy)

This layout does not have room for splitting across what I think are likely to be more useful boundaries. In particular, I think it'd be good to have at the toplevel whether the function is ISO C standard, POSIX standard, or a private extension.  Another thing that may be good to somehow express is a distinction separation between generally-portable "library-code-only" nonstandard functions, and libc functions which are explicitly exposing non-portable kernel-specific functionality. In the former would go things like `asprintf`, and in the latter would go things like `kqueue` or `epoll_create`.

That is, maybe something like:

  libc/
    src/
      c_std/{string/,math/,...}
      posix_std/{string/,math/,...}
      extensions_portable/{string/,stdlib/,...}
      extensions_macos/{string/,stdlib/,...}
      extensions_linux/{string/,stdlib/,...}

Probably also worth considering splitting the headers in the same way. E.g., move the current `libc/include/math.h` file to `libc/include/c_std/math.h`, and have a 'math.h' be generated by `%%include`ing the pieces from all the standards you wish to implement.



================
Comment at: libc/docs/implementation_standard.md:29
+
+namespace llvm_libc {
+
----------------
Unless all the symbols defined here are going to be suppressed via some external mechanism, this namespace name is also not okay, as it will break valid user programs using the un-reserved namespace "llvm_libc". 

I'd suggest renaming __llvm_libc.


Repository:
  rG LLVM Github Monorepo

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

https://reviews.llvm.org/D67867