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

Mon Oct 7 14:52:37 PDT 2019

abrachet added a comment.

In D67867#1692484 <https://reviews.llvm.org/D67867#1692484>, @MaskRay wrote:

> My earlier question is about why we need the namespace `__llvm_libc` at all. From `libc/src/string/strcat/strcat_test.cpp` I conclude it is for unit testing in an environment that already has a libc (gtest). This should probably be documented.
>
> Can we do things the other way round? No namespace, no `__llvm_libc_` prefix. Add the `-ffreestanding` compiler flag and just define `strstr, open, etc` in the global namespace. In unit tests, invoke `llvm-objcopy --redefine-syms=` to rename `strstr` to `__llvm_libc_strstr`, and call `__llvm_libc_strstr` in the tests. For functions that affect global states, gtest will not be suitable. It is good to think how the tests will be organized in the current early stage.

In D67867#1696908 <https://reviews.llvm.org/D67867#1696908>, @MaskRay wrote:

> The commit was done in a hurry. For the initial commit of a brand new project that sets up the project hierarchy, this seems to have received fewer than enough thumbs up. Many points raised in the review process were just shrugged off.

I don't know if it matters anymore because this was committed but I agree with @MaskRay. His suggestion of using `llvm-objcopy` to rename the symbols for tests makes much more sense to me. I haven't seen a libc that does testing in an ergonomic way and this suggestion seems the best to me, frankly.

There is a lot going on here, it's hard to follow it all in one patch, and I think some comments got lost because of this. I feel like a lot of big design decisions were made here, did I miss something on the libc-dev mailing list?

Repository:
  rL LLVM

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

https://reviews.llvm.org/D67867