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

[Fangrui Song via Phabricator via llvm-commits]

MaskRay added inline comments.


================
Comment at: libc/CMakeLists.txt:19
+
+set(AR "ar")
+
----------------
We can reuse CMAKE_AR.


================
Comment at: libc/include/ctype.h:16
+
+int isalnum(int c);
+
----------------
jyknight wrote:
> All the argument names should either be removed or put in the implementation namespace, e.g. named `__c`, etc. Since they're not specified by the standard, users can `#define` them to something else.
> 
> Removed is probably best.
> Removed is probably best.

+1


================
Comment at: libc/include/gcc_clang_size_t.h.in:1
+//===------- Definition of size_t for gcc and clang compilations --------*-===//
+//
----------------
jyknight wrote:
> It would be good to be clear as to who is responsible for providing which headers.
> 
> Right now, clang expects to provide `stddef.h`, and thus size_t, so this shouldn't be provided by the libc.
> 
> I note that some bsd libcs remove some of clang's provided headers, and other libcs like musl want their headers in the wrong order (libc first, then clang builtins), but I don't know why they do that, or why they want to do that. And I think that's probably not a good thing to do for the llvm libc.
See https://reviews.llvm.org/D65699#1614203 for why musl has a different search path order from glibc.

I agree we don't need gcc_clang_size_t.h.in for now.


================
Comment at: libc/include/math.h:18
+
+float acosf(float x);
+
----------------
I'd prefer deleting empty lines among similar functions.


================
Comment at: libc/include/null.h.in:21
+#else
+#define NULL ((void*)0)
+#endif // __cplusplus
----------------
dlj wrote:
> Do you think it would be reasonable to check for availability of __null, and use it if possible?
> 
> My reasoning: this would be different in Clang's AST, and that could lead to subtle differences (for example, when using the `nullPointerConstant` AST matcher).
The GNU extension is not necessary:

```
void foo(int a) { puts("int"); }
void foo(long a) { puts("long"); }
void foo(void* a) { puts("void*"); }
```

both `foo(__null)` and `foo(0L)` resolve to `foo(long)`.

C++03: an integral constant expression rvalue of integer type that evaluates to zero
C++11: an integer literal with value zero, or a prvalue of type std::nullptr_t

0L is clearly a conforming definition for all conforming compilers. This is also well tested by musl.


================
Comment at: libc/src/string/strcpy/strcpy.cpp:13
+namespace llvm_libc {
+
+
----------------
superfluous empty line.


================
Comment at: libc/src/string/strcpy/strcpy.cpp:16
+char *LLVM_LIBC_ENTRYPOINT(strcpy)(char *dest, const char *src) {
+  return reinterpret_cast<char*>(::memcpy(dest, src, ::strlen(src) + 1));
+}
----------------
Such delegation is inefficient. The call to memcpy will go through GOT/PLT. You need a hidden alias to memcpy to eliminate the PLT call.


Repository:
  rG LLVM Github Monorepo

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

https://reviews.llvm.org/D67867