[libc] Add a TableGen based header generator.

[sivachandra]

Summary

• The Python header generator has been removed.
• Docs giving a highlevel overview of the header gen scheme have been

added.

[sivachandra]

Use the new header generator to generate string.h.

[sivachandra]

Improve some comments.

[abrachet]

libc/spec/spec.td

13

'specified in' -> 'specified by' maybe?

libc/utils/HdrGen/Command.h

30

Why not llvm::Error?

libc/utils/HdrGen/Generator.cpp

23

Coding standards say to prefer static instead of anonymous namespaces.

26

sizeof(CommandPrefix) - 1 maybe? StringRef::StringRef(const char*) is constexpr though. You choose whichever you think is better.

42

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.

64

Can't this be done in place in Args instead of making two vectors and copying like this.

123

else if (!Line.startswith(CommentPrefix) {
    ...
}

is better than empty else if in my opinion.

libc/utils/HdrGen/Main.cpp

33

Nothing fancy is being done with it that it needs to be like this why not just for (auto &i : ReplacementValues)?

libc/utils/HdrGen/PublicAPICommand.cpp

17

Ditto about anonymous namespaces.

19

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?

79

using is prefered over typedef

92–106

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(...)

114

Don't we usually use llvm::Twine for these sorts of things not std::string?

185

Make it range based loop.

205

range based

217

range based

230

range based

libc/utils/HdrGen/PublicAPICommand.h

30

Is there a point to this? I can't seem to figure out why this is needed.

[abrachet]

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?

[sivachandra]

Address comments.

libc/utils/HdrGen/Command.h

30

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.

libc/utils/HdrGen/Generator.cpp

26

Personally, I feel -1 is confusing.

libc/utils/HdrGen/PublicAPICommand.cpp

19

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.

92–106

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.

114

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.

libc/utils/HdrGen/PublicAPICommand.h

30

The name has to be stored somewhere so I have chosen to put it with the command implementation. It is used in Generator::getCommandHandler.

[sivachandra]

In 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

[abrachet]

In D70197#1752985, @sivachandra wrote:

In 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.

libc/utils/HdrGen/Command.h

16–18

As a heads up these were added

30

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.

libc/utils/HdrGen/Generator.cpp

26

Fair enough. You could also make *Prefix as cosntexpr llvm::StringRef("string literal") if you like that.

libc/utils/HdrGen/Generator.h

2

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.

libc/utils/HdrGen/IncludeFileCommand.cpp

2

IncludeFileCommand

libc/utils/HdrGen/IncludeFileCommand.h

2

IncludeFileCommand.h if its even necessary, really we don't need the files name up there I think

libc/utils/HdrGen/Main.cpp

2

Wrong description

libc/utils/HdrGen/PublicAPICommand.cpp

92–106

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.

114

I didn't know that, thanks!

237

I think this was left by accident.

libc/utils/HdrGen/PublicAPICommand.h

2

Wrong description

[sivachandra]

Fix few copy-paste errors pointed out by abrachet; Get rid of Command::Status.

[sivachandra]

libc/utils/HdrGen/Command.h

30

I got rid of this class and introduced another one which encapsulates an SMLoc and SourceMgr.

[abrachet]

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?

libc/utils/HdrGen/PublicAPICommand.cpp

44

is '\r' a Windows thing? Is this necessary?

59

If you can be bothered '\n' is probably slightly preferred to "\n" there are a bunch of these.

61

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.

266

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.

[phosek]

libc/config/linux/api.td

78

nit: Should this be spelled as StdIOAPI since both IO and API are acronyms?

libc/docs/ground_truth_specification.rst

8

s/standanrd/standard/

libc/utils/HdrGen/Generator.cpp

39

LLVM style doesn't use { and } for blocks with a single statement. There are multiple instances of this throughout this patch.

62

Should this use && rather than and?

64

A doesn't seem to be used anywhere after this line?

[sivachandra]

Address latest round of comments.

[sivachandra]

libc/utils/HdrGen/Generator.cpp

62

Ah, sorry!! Effects of context switching between Python and C++.

64

A is a reference getting changed in place.

libc/utils/HdrGen/PublicAPICommand.cpp

44

Yes. But I did not test this on Windows so removed it now.

266

This is just forwarded to SourceMgr's PrintMessage. SourceMgr does its own pretty formatting so I think we should leave it as is.

[sivachandra]

In D70197#1754156, @abrachet wrote:

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 have now submitted it. Thanks a lot to you and to @phosek for reviewing.