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

[Siva Chandra via Phabricator via llvm-commits]

sivachandra added a comment.

I addressed most comments. For the rest, I asked for clarification/suggestion.



================
Comment at: libc/docs/header_generation.md:7
+approaches have served them well, parts of their systems have become extremely
+complicated making it hard to modify, extend or maintain. To avoid these
+problems in llvm-libc, we use a header generation mechanism. The mechanism is
----------------
dlj wrote:
> Unrequested opinion: my personal complaint in this area is that reading the files (as a human) requires understanding the entirety of the configuration macros, plus usually double-checking `clang -E -dM`, just to figure out what the preprocessor result will be. Generating files up-front at least carves out the internal configuration part.
> 
> I'm not sure how to word that eloquently, but it might be possible to say something a bit more concrete than the current wording.
I made starts to address this comment, but scrubbed them all out as I do not think I am doing a good job. Do you have any suggestion, or some specific points you want to see covered here?


================
Comment at: libc/docs/header_generation.md:16
+the file containing replacment commands should be `.h.def` instead of the normal
+`.h` extension. A header generation tool reads the `.h.def` files and produces
+a `.h` file. This tool does two things:
----------------
dlj wrote:
> It might be good to describe `.h.in` files, too.
> 
> (Obligatory bikeshed: and maybe give them a different extension to disambiguate from CMake-configured files?)
Changed the extension to ".h.inc". Also, added a note about the "begin" command and described the ".h.inc" files over there.


================
Comment at: libc/docs/header_generation.md:38
+
+## Parameter Syntax
+
----------------
dlj wrote:
> Wording nit: the distinction between "parameter" and "argument" caught me a bit by surprise: the "arguments" here are what I would call "parameters" -- things that parameterize the target; the term "parameters" here are free-floating values, but they could be passed as as what I would call "arguments." (This is mostly following the way these terms are used in C++: functions are parameterized, call expressions have arguments.)
> 
> I really don't want to get into a bikeshed argument here, though. If you don't agree with my reasoning, I can live with this as-is, too. :-)
I agree there was some mix up. I made minor changes now. How does it look?


================
Comment at: libc/include/null.h.in:21
+#else
+#define NULL ((void*)0)
+#endif // __cplusplus
----------------
MaskRay wrote:
> 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.
Removed this file for now.


================
Comment at: libc/scripts/gen_hdr.py:22
+INCLUDE_FILE_COMMAND = "include_file"
+
+
----------------
dlj wrote:
> It might be helpful to add a comment "command." For example, looking at the .h.in files, it's not immediately obvious that they are used for something other than CMake `configure_file`, so comments might help.
I have now changed the extension to ".h.inc". Also added a comment syntax for the ".h.def" files.

In a ".h.inc" file, there can be two kinds of comments:
1. Comments to describe what the file is for/about - Such comments can be put before the %%begin command. Anything goes before the %%begin command.
2. Comments which describe the actual contents - These comments should also go into the generated header file, so they should be normal C/C++ style comments.

On the other hand, one can have comments in the ".h.def" file attached to a command. Such comments should not be copied into the generated header file, so I added comment syntax for that: Prefix the comment lines with "<!>", then the rest of the line will be ignored.


================
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));
+}
----------------
MaskRay wrote:
> 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.
Since llvm-libc does not yet provide the memcpy implementation, we use ::memcpy from the system libc. When llvm-libc provides the memcpy implementation, this should be changed to a call to llvm_libc::memcpy.


Repository:
  rG LLVM Github Monorepo

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

https://reviews.llvm.org/D67867