[libc-commits] [PATCH] D89267: Move Cannonical Function List

[Siva Chandra via Phabricator via libc-commits]

sivachandra added a comment.

Still LGTM. Few more nitty inline comments.



================
Comment at: libc/utils/HdrGen/EntrypointsReader.cpp:28
+  if (!FileBuf)
+    llvm::PrintFatalError("File failed to load" + FilePath +
+                          " in EntrypointsReader\n");
----------------
If a statement ends up spanning multiple lines, then use braces for the enclosing `if`/`for` etc.


================
Comment at: libc/utils/HdrGen/EntrypointsReader.cpp:41
+  for (unsigned Count = 0; Count < Lines.size(); Count += 1) {
+    CurLine = Lines[Count].trim(' ').trim('\t');
+
----------------
Trim after dropping the comment? That is, after line #45?


================
Comment at: libc/utils/HdrGen/EntrypointsReader.cpp:43
+
+    if (CurLine.contains("#")) {
+      CurLine = CurLine.split("#").first;
----------------
No braces here.


================
Comment at: libc/utils/HdrGen/EntrypointsReader.cpp:47
+
+    if (CurLine.size() == 0) {
+      continue;
----------------
Ditto.


================
Comment at: libc/utils/HdrGen/EntrypointsReader.cpp:51
+
+    if (!InSet && CurLine.startswith("set(")) {
+      InSet = true;
----------------
This part of the code makes me nervous. Nothing wrong with you've got, but just that we are parsing a CMake file. We already "parse" the entrypoint list here: https://github.com/llvm/llvm-project/blob/master/libc/CMakeLists.txt#L76. So, technically we can just pass that list as a command line argument to hdrgen. I am not sure how portable that will be because of the command line length. Lets keep this for now. We can switch to command line args later if required.


================
Comment at: libc/utils/HdrGen/PublicAPICommand.cpp:92
     if (G.FunctionSpecMap.find(Name) == G.FunctionSpecMap.end())
-      llvm::PrintFatalError(Name + " not found in any standard spec.\n");
+      continue; // this used to be an error, but isn't anymore because
+                // we're getting the full function list from entrypoints.txt
----------------
michaelrj wrote:
> sivachandra wrote:
> > When running the `PublicAPICommand`, the `FunctionSpecMap` will be for the header file we are generating. So, it should still be an error if a function is not found, no?
> The `FunctionSpecMap` is indeed for the header file we're generating, but that line is checking if each function in the list `Functions` is in `FunctionSpecMap`. Previously the list in `Functions` (or `G.Functions`) was supposed to be identical to that list, but since we're now ignoring what header file we're looking for in `getFunctionsFromEntrypointsFile`, the list `Functions` contains every function listed in `entrypoints.txt`. When I run the  code with this as an error it crashes when trying to load a header file because not all functions listed are in that header file.
Ah, got it. Thanks for explaining. Instead of a dangling comment, can you format like this:

```
if (...) {
  // comment
  // comment
  // ...
  continue;
}
```

Also, try to use correct grammar like beginning sentences with **U**pper case words. You don't need to explain previous code (in fact, it will be confusing if you do) but explain the current code.


Repository:
  rG LLVM Github Monorepo

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

https://reviews.llvm.org/D89267