[libc-commits] [PATCH] D80832: [libc] Expose APIGenerator.

[Siva Chandra via Phabricator via libc-commits]

sivachandra added a comment.

Mostly LGTM with few minor comments.



================
Comment at: libc/utils/HdrGen/PublicAPICommand.cpp:185
 
-public:
-  APIGenerator(llvm::StringRef Header, llvm::RecordKeeper &Records)
-      : StdHeader(Header) {
-    index(Records);
-  }
+void APIGenerator::write(llvm::raw_ostream &OS) {
+  for (auto &Pair : MacroDefsMap) {
----------------
After renaming the class to APIIndexer, the write method should be decoupled from the class and given a name say `writeAPIFromIndex` or something. AFAICT, it only needs access to the members you have made public in this patch.


================
Comment at: libc/utils/HdrGen/PublicAPICommand.h:28
 
+using NameToRecordMapping = std::unordered_map<std::string, llvm::Record *>;
+using NameSet = std::unordered_set<std::string>;
----------------
These names are generic enough that they should be nested in the class. You can make them public if you want.


================
Comment at: libc/utils/HdrGen/PublicAPICommand.h:42
 
+class APIGenerator {
+private:
----------------
At this point, it probably makes sense to call this class APIIndexer?


================
Comment at: libc/utils/HdrGen/PublicAPICommand.h:68
+public:
+  explicit APIGenerator(llvm::RecordKeeper &Records) : StdHeader(llvm::None) {
+    index(Records);
----------------
Can you add comments explaining the difference between the two constructors?


================
Comment at: libc/utils/HdrGen/PublicAPICommand.h:70
+    index(Records);
+  }
+  APIGenerator(llvm::StringRef Header, llvm::RecordKeeper &Records)
----------------
Empty line after this?


Repository:
  rG LLVM Github Monorepo

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

https://reviews.llvm.org/D80832