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

[Siva Chandra via Phabricator via libc-commits]

sivachandra added inline comments.


================
Comment at: libc/utils/HdrGen/Command.h:49
+                   llvm::StringRef StdHeader,
+                   std::vector<std::string> EntrypointNameList,
+                   llvm::RecordKeeper &Records,
----------------
Instead of passing the entrypoint list to all commands, can perhaps pass it to the PublicAPICommand constructor here: https://github.com/llvm/llvm-project/blob/master/libc/utils/HdrGen/Generator.cpp#L43


================
Comment at: libc/utils/HdrGen/Main.cpp:28
+llvm::cl::list<std::string>
+    EntrypointNamesOption("ent", llvm::cl::desc("<list of entrypoints>"),
+                          llvm::cl::OneOrMore);
----------------
`desc` should be proper English description. `value_desc` can be what you have listed as `<list of entrypoints>`.

Also, to be consistent with this https://github.com/llvm/llvm-project/blob/master/libc/utils/HdrGen/PrototypeTestGen/PrototypeTestGen.cpp, use just `e` for the option name.


================
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
----------------
sivachandra wrote:
> 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.
I think you have not addressed this?


Repository:
  rG LLVM Github Monorepo

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

https://reviews.llvm.org/D89267