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

[Alex Brachet via Phabricator via libc-commits]

abrachet accepted this revision.
abrachet added a comment.
This revision is now accepted and ready to land.

I think this is in a good spot, it's easy to read and modify later as needs change. I don't know much about TableGen, I've tried my best to look through, but it would be preferable if other reviewers would be able to take a look too. Up to your discretion how long you would like to wait to let others chime in. LGTM

I think that incorporating `clang-format` down the line in the header generation process isn't a bad idea. For one it would get rid of that strange white space (if we can't find the cause) and would also get rid of inconsistencies of the placement of `*`, `char * strtok(char *__restrict, const char *__restrict);`. Certainly it doesn't matter for now, just something to think about.

Also of note, I find a lot of libc's decide to put `#pragma GCC system_header`. Do you have thoughts on this?



================
Comment at: libc/utils/HdrGen/PublicAPICommand.cpp:43
+  llvm::SmallVector<llvm::StringRef, 10> Lines;
+  llvm::SplitString(Text, Lines, "\n\r");
+  size_t shortest_indent = 1024;
----------------
is '\r' a Windows thing? Is this necessary?


================
Comment at: libc/utils/HdrGen/PublicAPICommand.cpp:58
+    if (L.size() >= shortest_indent) {
+      OS << L.drop_front(shortest_indent) << "\n";
+    } else {
----------------
If you can be bothered `'\n'` is probably slightly preferred to `"\n"` there are a bunch of these.


================
Comment at: libc/utils/HdrGen/PublicAPICommand.cpp:60
+    } else {
+      OS << L << "\n";
+    }
----------------
I would get rid of this "\n" because it puts two spaces which is too much I think.
```
#define __need_NULL
#include <stddef.h>
  

#define __need_size_t
#include <stddef.h>
 

```
Also, I couldn't find out the cause of this, but this emits an extra space character on the first new line. I don't know how much formatting matters for the headers necessarily, but a random space is enough to try to do something about it.


================
Comment at: libc/utils/HdrGen/PublicAPICommand.cpp:265
+  if (Args.size() != 0) {
+    Reporter.printFatalError("public_api command does not take any arguments.");
+  }
----------------
Will `llvm::SourceMgr::printMessage` print a newline? Some `llvm::PrintFatalError` calls have a newline at the end, would be good to have consistency, even if this isn't a user facing tool.


Repository:
  rG LLVM Github Monorepo

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

https://reviews.llvm.org/D70197