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

[Siva Chandra via Phabricator via libc-commits]

sivachandra added inline comments.


================
Comment at: libc/cmake/modules/LLVMLibCHeaderRules.cmake:91
             --def ${in_file} ${replacement_params} -I ${LIBC_SOURCE_DIR}
+	    --entrypoints ${LIBC_SOURCE_DIR}/config/${LIBC_TARGET_OS}/${LIBC_TARGET_MACHINE}/entrypoints.txt
             ${LIBC_SOURCE_DIR}/config/${LIBC_TARGET_OS}/api.td
----------------
Align with the other lines in the command?


================
Comment at: libc/spec/llvm_libc_ext.td:1
 def LLVMLibcExt : StandardSpec<"llvm_libc_ext"> {
   HeaderSpec String = HeaderSpec<
----------------
Can you move the changes in this file to a separate patch?


================
Comment at: libc/spec/spec.td:43
 def FloatType : NamedType<"float">;
 def DoubleType : NamedType<"double">;
 def LongDoubleType : NamedType<"long double">;
----------------
Changes in this file and `stdc.td` can also go into a separate patch.


================
Comment at: libc/spec/spec.td:56
 
+//comparison for reference
+//def ConstVoidPtr : ConstType<VoidPtr>;
----------------
Remove this comment?


================
Comment at: libc/spec/stdc.td:4
+  //all other types are defined in spec.td, but these ones would collide with
+  //the definition of FILE in libc/config/public_api.td, so this one gets to be scoped.
   NamedType FILE = NamedType<"FILE">;
----------------
I did not get this comment. Can you explain a bit more?


================
Comment at: libc/utils/HdrGen/EntrypointsLoader.cpp:29
+    llvm::PrintFatalError(
+        "File failed to load entrypoints in EntrypointsLoader\n");
+
----------------
The error message should include the path to the file.


================
Comment at: libc/utils/HdrGen/EntrypointsLoader.cpp:31
+
+  EntrypointsLoader::NameSet FunctionSet = *(new EntrypointsLoader::NameSet());
+  std::string HeaderNameStripped = std::string(HeaderName.split(".").first);
----------------
Why is a `new` required here? Also, ins't it a memory leak? Isn't a simple stack variable sufficient?


================
Comment at: libc/utils/HdrGen/EntrypointsLoader.cpp:62
+        // and return just "isalnum"
+        llvm::StringRef FuncHeaderName = CurLine.rsplit(".").first;
+        // and this should take the same line and return just "ctype"
----------------
We should not parse the header name here. Instead, we can use the spec to map the functions to the header file in the caller.


================
Comment at: libc/utils/HdrGen/EntrypointsLoader.h:1
+//===-- A class to index functions listed in entrypoints files --*- C++ -*-===//
+//
----------------
Call this file (and the `.cpp` file) `EntrypointsReader.[h|cpp]`?


================
Comment at: libc/utils/HdrGen/EntrypointsLoader.h:19
+
+class EntrypointsLoader {
+private:
----------------
Seems like this class is not of much use. Remove it?


================
Comment at: libc/utils/HdrGen/EntrypointsLoader.h:26
+EntrypointsLoader::NameSet
+getFunctionsFromEntrypoints(std::string FilePath, llvm::StringRef HeaderName);
+} // namespace llvm_libc
----------------
Call this function `getFunctionsFromEntrypointsFile`? Also, we don't need to pass the `HeaderName`. Just get all the functions and let the caller pick the function corresponding to the header file.


Repository:
  rG LLVM Github Monorepo

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

https://reviews.llvm.org/D89267