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

[Siva Chandra via Phabricator via libc-commits]

sivachandra added inline comments.


================
Comment at: libc/utils/HdrGen/Command.h:29
+public:
+  class Status {
+    bool Success;
----------------
abrachet wrote:
> Why not `llvm::Error`?
IIUC, using llvm::Error will require defining error codes etc. I feel that is an overkill for the use case here which just requires a pass/fail status + a message.


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


================
Comment at: libc/utils/HdrGen/PublicAPICommand.cpp:18
+
+const char NamedTypeClassName[] = "NamedType";
+const char PtrTypeClassName[] = "PtrType";
----------------
abrachet wrote:
> 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?
There are names of classes defined in spec.td. I think it is easy to look them up, and also cleaner, if they are stored in a variable this way and not inlined.


================
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); }
----------------
abrachet wrote:
> 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(...)`
I am not sure we can do something like that as all of them are of the same type. We can introduce an enum though, but I feel it is an overkill.


================
Comment at: libc/utils/HdrGen/PublicAPICommand.cpp:113
+    } else if (isaPtrType(TypeRecord)) {
+      return getTypeAsString(TypeRecord->getValueAsDef("PointeeType")) + " *";
+    } else if (isaConstType(TypeRecord)) {
----------------
abrachet wrote:
> Don't we usually use `llvm::Twine` for these sorts of things not `std::string`?
Yes, something like Twine would have been good here. However, we cannot use Twine itself as it stores references to its components. Since this function is called recursively, we end up stack-use-after-scope errors.


================
Comment at: libc/utils/HdrGen/PublicAPICommand.h:29
+public:
+  static const char Name[];
+
----------------
abrachet wrote:
> Is there a point to this? I can't seem to figure out why this is needed.
The name has to be stored somewhere so I have chosen to put it with the command implementation. It is used in Generator::getCommandHandler.


Repository:
  rG LLVM Github Monorepo

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

https://reviews.llvm.org/D70197