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

[Fangrui Song via Phabricator via llvm-commits]

MaskRay added inline comments.


================
Comment at: libc/cmake/modules/LLVMLibCRules.cmake:128
+    PRIVATE
+      -fpie -std=c++11
+  )
----------------
there is an existing cmake variable to specify this
llvm has moved to c++14 now.


================
Comment at: libc/cmake/modules/LLVMLibCRules.cmake:157
+    OUTPUT ${object_file}
+    DEPENDS ${object_file_raw} llvm-objcopy
+    COMMAND $<TARGET_FILE:llvm-objcopy> --add-symbol "${target_name}=.llvm.libc.entrypoint.${target_name}:0,function" ${object_file_raw} ${object_file}
----------------
Prefer CMAKE_OBJCOPY. Does this target need llvm-objcopy to build?


================
Comment at: libc/include/string.h:56
+
+void *memset(void *, int, size_t);
+
----------------
place mem* functions together


================
Comment at: libc/src/string/strcat/strcat.h:13
+#include <string.h>
+namespace __llvm_libc {
+
----------------
blank line above namespace


================
Comment at: libc/src/string/strcat/strcat.h:15
+
+char *strcat(char *dest, const char *src);
+
----------------
delete dest and src


================
Comment at: libc/src/string/strcpy/strcpy_test.cpp:18
+
+  char* result = __llvm_libc::strcpy(dest, abc.c_str());
+  ASSERT_EQ(dest, result);
----------------
clang-format


================
Comment at: libc/src/string/strcpy/strcpy_test.cpp:23
+
+  delete [] dest;
+}
----------------
clang-format.

This should be `delete[] dest`;


================
Comment at: llvm/CMakeLists.txt:62
 # one for llvm+clang+... using the same sources.
-set(LLVM_ALL_PROJECTS "clang;clang-tools-extra;compiler-rt;debuginfo-tests;libclc;libcxx;libcxxabi;libunwind;lld;lldb;llgo;openmp;parallel-libs;polly;pstl")
+set(LLVM_ALL_PROJECTS "clang;clang-tools-extra;compiler-rt;debuginfo-tests;libc;libclc;libcxx;libcxxabi;libunwind;lld;lldb;llgo;openmp;parallel-libs;polly;pstl")
 set(LLVM_ENABLE_PROJECTS "" CACHE STRING
----------------
Making it build by default makes me concerned: there are some cmake constructs which I am not sure build on a non-Linux platform. If nobody has tested that, probably hold off on this a bit.


Repository:
  rG LLVM Github Monorepo

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

https://reviews.llvm.org/D67867