[libc-commits] [PATCH] D70197: [libc] Add a TableGen based header generator.

[Alex Brachet via Phabricator via libc-commits]

abrachet added inline comments.


================
Comment at: libc/spec/spec.td:12
+
+// Class to describe concrete structs specified in the standard.
+class Struct<string name> : NamedType<name> {
----------------
'specified in' -> 'specified by' maybe?


================
Comment at: libc/utils/HdrGen/Command.h:29
+public:
+  class Status {
+    bool Success;
----------------
Why not `llvm::Error`?


================
Comment at: libc/utils/HdrGen/Generator.cpp:22
+
+namespace {
+
----------------
[[ https://llvm.org/docs/CodingStandards.html#anonymous-namespaces | Coding standards ]] say to prefer static instead of anonymous namespaces.


================
Comment at: libc/utils/HdrGen/Generator.cpp:25
+const char CommandPrefix[] = "%%";
+const size_t CommandPrefixSize = llvm::StringRef(CommandPrefix).size();
+
----------------
`sizeof(CommandPrefix) - 1` maybe? `StringRef::StringRef(const char*)` is constexpr though. You choose whichever you think is better.


================
Comment at: libc/utils/HdrGen/Generator.cpp:41
+    if (!IncludeFileCmd) {
+      IncludeFileCmd.reset(new IncludeFileCommand);
+    }
----------------
`IncludeFileCmd = std::make_unique<IncludeFileCommand>()` would be preferred I believe. There shouldn't be a need to call reset because it doesn't have a managed object here.


================
Comment at: libc/utils/HdrGen/Generator.cpp:63
+
+  for (llvm::StringRef Raw : RawArgs) {
+    llvm::StringRef Arg = Raw.trim(' ');
----------------
Can't this be done in place in `Args` instead of making two vectors and copying like this.


================
Comment at: libc/utils/HdrGen/Generator.cpp:122
+      ; // Just drop the line.
+    } else {
+      // There is no comment or command on this line so we just write it as is.
----------------
```
else if (!Line.startswith(CommentPrefix) {
    ...
}
```
is better than empty `else if` in my opinion.


================
Comment at: libc/utils/HdrGen/Main.cpp:32
+void ParseArgValuePairs(std::unordered_map<std::string, std::string> &Map) {
+  for (auto it = ReplacementValues.begin(); it != ReplacementValues.end();
+       ++it) {
----------------
Nothing fancy is being done with `it` that it needs to be like this why not just `for (auto &i : ReplacementValues)`?


================
Comment at: libc/utils/HdrGen/PublicAPICommand.cpp:16
+
+namespace {
+
----------------
Ditto about anonymous namespaces.


================
Comment at: libc/utils/HdrGen/PublicAPICommand.cpp:18
+
+const char NamedTypeClassName[] = "NamedType";
+const char PtrTypeClassName[] = "PtrType";
----------------
These are strange to me, *TypeClassName are each used once and are not any easier to read than just using the string literal itself, why the extra identifier here?


================
Comment at: libc/utils/HdrGen/PublicAPICommand.cpp:78
+
+  typedef std::unordered_map<std::string, llvm::Record *> NameToRecordMapping;
+  typedef std::unordered_set<std::string> NameSet;
----------------
`using` is prefered over `typedef`


================
Comment at: libc/utils/HdrGen/PublicAPICommand.cpp:91-105
+  bool isaNamedType(llvm::Record *Def) { return isa(Def, NamedTypeClass); }
+
+  bool isaStructType(llvm::Record *Def) { return isa(Def, StructClass); }
+
+  bool isaPtrType(llvm::Record *Def) { return isa(Def, PtrTypeClass); }
+
+  bool isaConstType(llvm::Record *Def) { return isa(Def, ConstTypeClass); }
----------------
These could be templatized in some way
``` 
template <llvm::Record *Record>
bool isa(llvm::Record *Def) { return isa(Def, Record); }
```
I personally prefere `isa<NamedTypeClass>(...)` to `isaNamedType(...)`


================
Comment at: libc/utils/HdrGen/PublicAPICommand.cpp:113
+    } else if (isaPtrType(TypeRecord)) {
+      return getTypeAsString(TypeRecord->getValueAsDef("PointeeType")) + " *";
+    } else if (isaConstType(TypeRecord)) {
----------------
Don't we usually use `llvm::Twine` for these sorts of things not `std::string`?


================
Comment at: libc/utils/HdrGen/PublicAPICommand.cpp:184
+    const auto &DefsMap = Records.getDefs();
+    for (auto it = DefsMap.begin(); it != DefsMap.end(); ++it) {
+      llvm::Record *Def = it->second.get();
----------------
Make it range based loop.


================
Comment at: libc/utils/HdrGen/PublicAPICommand.cpp:204
+  void write(llvm::raw_ostream &OS) {
+    for (auto it = MacroDefsMap.begin(); it != MacroDefsMap.end(); ++it) {
+      const std::string &Name = it->first;
----------------
range based


================
Comment at: libc/utils/HdrGen/PublicAPICommand.cpp:216
+
+    for (auto it = TypeDeclsMap.begin(); it != TypeDeclsMap.end(); ++it) {
+      const std::string &Name = it->first;
----------------
range based


================
Comment at: libc/utils/HdrGen/PublicAPICommand.cpp:229
+    OS << "__BEGIN_C_DECLS\n\n";
+    for (auto it = Functions.begin(); it != Functions.end(); ++it) {
+      const std::string &Name = *it;
----------------
range based


================
Comment at: libc/utils/HdrGen/PublicAPICommand.h:29
+public:
+  static const char Name[];
+
----------------
Is there a point to this? I can't seem to figure out why this is needed.


Repository:
  rG LLVM Github Monorepo

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

https://reviews.llvm.org/D70197