- The Python header generator has been removed.
- Docs giving a highlevel overview of the header gen scheme have been
added.
Details
Details
Diff Detail
Diff Detail
- Repository
- rG LLVM Github Monorepo
- Build Status
Buildable 40914 Build 41056: arc lint + arc unit
Event Timeline
| 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. | |
Comment Actions
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?
Comment Actions
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. | |
Comment Actions
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 | |
| libc/utils/HdrGen/Command.h | ||
|---|---|---|
| 30 | I got rid of this class and introduced another one which encapsulates an SMLoc and SourceMgr. | |
Comment Actions
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. | |
| libc/config/linux/api.td | ||
|---|---|---|
| 77 | nit: Should this be spelled as StdIOAPI since both IO and API are acronyms? | |
| libc/docs/ground_truth_specification.rst | ||
| 7 | 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? | |
| 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. | |
nit: Should this be spelled as StdIOAPI since both IO and API are acronyms?