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

Alex Brachet via Phabricator via libc-commits libc-commits at lists.llvm.org
Wed Nov 20 14:17:16 PST 2019

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.

  rG LLVM Github Monorepo



More information about the libc-commits mailing list