[libc-commits] [PATCH] D79192: [libc] Add integration tests.

[Alex Brachet via Phabricator via libc-commits]

abrachet added inline comments.


================
Comment at: libc/utils/HdrGen/PrototypeTestGen/PrototypeTestGen.cpp:24
+
+namespace llvm_libc {
+
----------------
Is there a reason for this to be namespaced? Should we use `__llvm_libc` instead?


================
Comment at: libc/utils/HdrGen/PrototypeTestGen/PrototypeTestGen.cpp:27
+bool TestGeneratorMain(llvm::raw_ostream &OS, llvm::RecordKeeper &records) {
+  // Print includes
+  OS << "#include \"TypeTraits.h\"\n";
----------------
Unecessary.


================
Comment at: libc/utils/HdrGen/PrototypeTestGen/PrototypeTestGen.cpp:30-32
+  for (const auto &header : G->PublicHeaders) {
+    OS << "#include <" << header << ">\n";
+  }
----------------
Don't need the brackets


================
Comment at: libc/utils/HdrGen/PrototypeTestGen/PrototypeTestGen.cpp:36-41
+    if (G->FunctionSpecMap.count(entrypoint) == 0) {
+      llvm::errs() << "ERROR: entrypoint '" << entrypoint
+                   << "' could not be found in spec in any public header\n";
+      return true;
+    }
+    llvm::Record *functionSpec = G->FunctionSpecMap[entrypoint];
----------------
These could be combined to 
```lang=CPP
auto it = G->FunctionSpecMap.find(entrypoint);
if (it == G->FunctionSpecMap.end()) {} // error
llvm::Record *functionSpec = *it;
```
so as to not search twice.


================
Comment at: libc/utils/HdrGen/PrototypeTestGen/PrototypeTestGen.cpp:45-48
+    // _Noreturn is an indication for the compiler that a function
+    // doesn't return, and isn't a type understood by c++ templates.
+    if (returnType.contains("_Noreturn"))
+      returnType = "void";
----------------
`_Noreturn` isn't a C++ keyword, otherwise the type system shouldn't care about attributes no matter where they show up. I think instead of removing these manually we should just include `__llvm-libc-common.h` which will define this as `[[noreturn]]` we might in the future use other macros in type names. 


================
Comment at: libc/utils/HdrGen/PrototypeTestGen/PrototypeTestGen.cpp:52
+    auto args = functionSpec->getValueAsListOfDefs("Args");
+    for (size_t i = 0; i < args.size(); ++i) {
+      llvm::Record *argType = args[i]->getValueAsDef("ArgType");
----------------
`for (size_t i = 0, size = args.size(); i < size; ++i)`? I believe the code standards prefers this


================
Comment at: libc/utils/HdrGen/PrototypeTestGen/PrototypeTestGen.cpp:75
+  llvm::cl::ParseCommandLineOptions(argc, argv);
+  return TableGenMain(argv[0], &llvm_libc::TestGeneratorMain);
+}
----------------
`&` not necessary, I believe.


================
Comment at: libc/utils/HdrGen/PublicAPICommand.cpp:111
+      if (!StdHeader.hasValue() || Header == StdHeader) {
+        PublicHeaders.insert(std::string(Header));
         auto MacroSpecList = HeaderSpec->getValueAsListOfDefs("Macros");
----------------
`PublicHeaders.emplace(Header)`


================
Comment at: libc/utils/HdrGen/PublicAPICommand.cpp:187
 public:
+  APIGenerator(llvm::RecordKeeper &Records) : StdHeader(llvm::None) {
+    index(Records);
----------------
`explicit` maybe?


================
Comment at: libc/utils/HdrGen/PublicAPICommand.cpp:280-282
+std::unique_ptr<APIReader> makeAPIReader(llvm::RecordKeeper &Records) {
+  return std::make_unique<APIGenerator>(Records);
+}
----------------
Is this simpler than just using `std::make_unique<APIGenerator>(Records);` wherever it would be used?


Repository:
  rG LLVM Github Monorepo

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

https://reviews.llvm.org/D79192