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

[David L. Jones via Phabricator via llvm-commits]

dlj added a comment.

After a quick once-over, I do have a few nit-picky comments. I plan to do another pass, but these are some things that were a little bit surprising up front.



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


================
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:
----------------
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?)


================
Comment at: libc/docs/header_generation.md:38
+
+## Parameter Syntax
+
----------------
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. :-)


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


================
Comment at: libc/scripts/gen_hdr.py:22
+INCLUDE_FILE_COMMAND = "include_file"
+
+
----------------
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.


================
Comment at: libc/scripts/gen_hdr.py:37
+
+    def __enter__(self):
+        if self.filename is None:
----------------
(Side note: this looks like it could be a function with `@contextlib.contextmanager` instead, which might be simpler: https://docs.python.org/3/library/contextlib.html#contextlib.contextmanager)


================
Comment at: libc/scripts/gen_hdr.py:71
+
+def _report_error(loc, msg):
+    sys.exit("ERROR:%s: %s" % (loc, msg))
----------------
Nit: maybe `_fatal_error` instead? It seems reasonable to assume `_report_error` would only report.


Repository:
  rG LLVM Github Monorepo

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

https://reviews.llvm.org/D67867