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

[Siva Chandra via Phabricator via llvm-commits]

sivachandra added a comment.

In D67867#1682894 <https://reviews.llvm.org/D67867#1682894>, @jyknight wrote:

> 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)


My choice here is driven by my preference to keep tests co-located with the implementation. Since we are in general going to have different kinds of testing like unit-testing, differential testing, fuzz-testing, I have chosen to put implementations of individual functions in their own directories. This way, the tests (which will span multiple files) will all be in a single directory and avoid clutter in the higher level directory. In the rare occasion that implementation also spans multiple files, all the implementation files will also live together.

That said, I think we will need a place for common/shared infrastructure. For example, if all hyperbolic functions use a common algorithm, that algorithm should probably have its own target and go in a directory like src/math/__<appropriate_name>.

I agree that, they way I have structured it now, it can be an annoyance. But, I am of the opinion that it gives us a simpler mental model, and that the annoyance can be overcome with some kind of convenience tooling/scripting if required.

> 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/,...}

I did consider such a layout. However, the same kind of distinction can be achieved by two things:

1. Next to the implementation and/or target listing for a function, call out the standard/extension that prescribes it.
2. When composing the target for libc.a for a platform, group the functions per standards/extensions they come from.

Another point which made me not pick this layout: I agree that as a catalog, the structure you are suggesting could be more meaningful. But as a developer, my mental model is much simpler if all the functions from a header file are grouped in one place, irrespective of the standard they come from.

> 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.

Irrespective of the directory layout, I think we will have such %%include-ed files anyway. For example, linux extensions can only be %%include-ed for linux targets.



================
Comment at: libc/docs/implementation_standard.md:29
+
+namespace llvm_libc {
+
----------------
jyknight wrote:
> 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.
Thanks again for pointing out another of my oversights. I will change per you suggestion when I update the code next.


Repository:
  rG LLVM Github Monorepo

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

https://reviews.llvm.org/D67867