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

[Alex Brachet via Phabricator via libc-commits]

abrachet marked 2 inline comments as done.
abrachet added a comment.

In D70197#1752985 <https://reviews.llvm.org/D70197#1752985>, @sivachandra wrote:

> In D70197#1752533 <https://reviews.llvm.org/D70197#1752533>, @abrachet wrote:
>
> > Not of huge importance, more of curiosity, but could you do what you did with a past revision where you pushed a review with an svg file to your fork so we could see what it looks like on your GitHub?
>
>
> Uploaded here: https://github.com/sivachandra/llvm-libc/blob/master/docs/header_gen_scheme.svg


It looks quite nice. I've said this before I believe, but I really like the focus on documentation. Its pretty crucial for what were trying to accomplish with the modularity of this libc and the changes that brings to the build process and I think its great so far.



================
Comment at: libc/utils/HdrGen/Command.h:15-17
+#include "llvm/Support/Error.h"
+
+#include <system_error>
----------------
As a heads up these were added


================
Comment at: libc/utils/HdrGen/Command.h:29
+public:
+  class Status {
+    bool Success;
----------------
sivachandra wrote:
> 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.
I think adding those includes is worth it because it increases readability, people are very familiar with `llvm::Error` and won't need to go searching for what `Status` is like I had to. Up to you.


================
Comment at: libc/utils/HdrGen/Generator.cpp:25
+const char CommandPrefix[] = "%%";
+const size_t CommandPrefixSize = llvm::StringRef(CommandPrefix).size();
+
----------------
sivachandra wrote:
> 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.
Fair enough. You could also make *Prefix as `cosntexpr llvm::StringRef("string literal")` if you like that.


================
Comment at: libc/utils/HdrGen/Generator.h:1
+//===------------ Generator.h - The main header generation class ----------===//
+//
----------------
I believe the convention in LLVM is to put `-*- C++ -*- ` in the header for header files so text editors can know if it is a C or C++ header.


================
Comment at: libc/utils/HdrGen/IncludeFileCommand.cpp:1
+//===------------------ Implementation of IncludeCommand ------------------===//
+//
----------------
IncludeFileCommand


================
Comment at: libc/utils/HdrGen/IncludeFileCommand.h:1
+//===-- IncludeCommand.h - Class which implements the %%include command ---===//
+//
----------------
IncludeFileCommand.h if its even necessary, really we don't need the files name up there I think


================
Comment at: libc/utils/HdrGen/Main.cpp:1
+//===---------------- C standard library header string.h ------------------===//
+//
----------------
Wrong description


================
Comment at: libc/utils/HdrGen/PublicAPICommand.cpp:236
+
+      //std::string ReturnTypeString = getTypeAsString(ReturnType);
+      OS << getTypeAsString(ReturnType) << " " << Name << "(";
----------------
I think this was left by accident.


================
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); }
----------------
sivachandra wrote:
> 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.
Right I wasn't suggesting that extreme, just using a `llvm::Record *` in the template parameter, but that's realistically no more readable than what is currently there. Disregard that.


================
Comment at: libc/utils/HdrGen/PublicAPICommand.cpp:113
+    } else if (isaPtrType(TypeRecord)) {
+      return getTypeAsString(TypeRecord->getValueAsDef("PointeeType")) + " *";
+    } else if (isaConstType(TypeRecord)) {
----------------
sivachandra wrote:
> 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.
I didn't know that, thanks!


================
Comment at: libc/utils/HdrGen/PublicAPICommand.h:1
+//===- IncludeCommand.h - Class which implements the %%public_api command -===//
+//
----------------
Wrong description


Repository:
  rG LLVM Github Monorepo

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

https://reviews.llvm.org/D70197