Adds a TypeDef class to OpBase and backing generation code
Seems like TypeDef could just provide a range of it's parameters and callers of this could just you llvm::map_range instead.
nit: Drop all of the mlir:: in this file, and throughout the revision.
The definitions of methods in header files should be fully qualified :: and not placed in a namespace in the .cpp.
nit: Prefer early return instead.
nit: Unless the end loop iterator changes, cache it.
The name of this parameter doesn't seem to match what you are doing here.
Could you just format the parameters into a llvm::raw_string_ostream instead of constructing separate strings?
Same comment as above.
This looks like the indent is off.
Do we need to generate this method? Seems like the uses of it by the generator could just inline the string instead.
nit: Spell out auto here, also use a reference instead of by-value.
Seems like this would be something only generated in the source file and marked as static.
This looks over indented, should be 2 spaces not 4.
This looks misaligned.
nit: Spell out auto here and don't use a by-value iterator variable.
Should these be auto &instead?
Please remove all of the personal comments from the revision.
nit: Comments should be complete sentences. This applies throughout the revision.
nit: Same comment about auto here as above.
Can we use a LogicalResult return instead?
Can we avoid generating print/parse on the types themselves? We should try to keep as much internal details hidden away as possible.
As for declaring parse(...), print(...), and getMnemonic() in the header file: these are intended for cases where (for some reason) the dialect doesn't use the global parse/print dispatch method. The dialect could call each parse/print method in its Dialect::parseType/Dialect::printType methods, in concert with the getMnemonic(). I figure even if it's not often used, its just an extra 3 lines in each type declaration.
Do you agree or should I remove it?
Ah. I thought proper punctuation was only necessary after full sentences.
Really great stuff, I'm glad you've been pushing this! Mostly nits left for the code, it's enough that we can just iterate further in tree. Can you document all of this cool new stuff? Either in docs/OpDefinitions or a new docs/TypeDefinitions?
nit: Can we remove the llvm:: on these as well?
Is the llvm:: here necessary?
Please drop else after a return.
nit: Drop the != nullptr.
Please cache the end iterator of loops if the size doesn't change.
The formatting here seems off.
nit: Drop trivial braces here. Also if you use braces on one if/else, please use it for all of them.
Please only use namespaces in source files for classes.
nit: Drop the newline here.
nit: Drop trivial braces here.
Please use braces on every else if they are used on one.
nit: auto -> const TypeDef &
Can you stream the tgfmt directly into os?
Can you stream it into os directly?
I'll update the documentation as you requested. Do you mind if I do it in a subsequent commit? I have a feeling there's going to be some back-and-forth on the documentation and I don't want that editorial process to gate this commit.
Meanwhile, I've addressed all your comments. No matter how carefully I examine my code, I just keep missing your nits. I gotta learn to write code in the LLVM style. It's pretty much the complete opposite of how I'm used to writing code!
Actually, no. It doesn't find Optional if I remove it. I'd have to add a 'using namespace' in the header, which is a no-no.
Why StringRef and ArrayRef work and nothing else in the llvm namespace is a mystery to me, though my understanding of C++ namespace scoping rules is doubtlessly lacking.
Doesn't work in this case. Since FieldInfo is in the mlir namespace (with the rest of the Test dialect), these two functions have to be also. They do not have to be exposed outside of this file, so they're not declared in the header.
Ohhh. I'll do ya one better. Lemme know if the is too far deep into C++ trickery territory.
That's fine with me.
LGTM after fixing the remaining comments.
Optional, like ArrayRef/StringRef/and many other things, is already re-exported into the MLIR namespace.
You need to fully qualify the methods: https://llvm.org/docs/CodingStandards.html#use-namespace-qualifiers-to-implement-previously-declared-functions
You should very very rarely (most cases are bugs in the compiler, e.g. some templates in GCC 5) need to actually use a namespace in the .cpp to define a previously declared method.
nit: Can you please avoid using namespace llvm here?
nit: Add & unless it is trivially copyable, using the type instead of auto would it make it a bit clearer what is being iterated here.
I would prefer that these just be function_ref and boolean fields of the class instead.
This class needs to be wrapped in an anonymous namespace.
nit: Prefer having the members at the end of the class definition, not first.
Please do not use typedef, prefer using Foo = blah; directives instead.
That kinda defeats the syntactic niceness. Will convert this to normal class w/ enum to determine the format. This will make the calling syntax longer but preserve the efficiency.
- Rebasing w/ conflicts
- New branch, applying everything at once
- Clang-(format|tidy) fixes
- One more clang-tidy fix
- Changes based on Chris' comments
- Ending a sentence comment w/ a period
- Revisions base on River's comments
- Clang-tidy and some missed comments
- Missed a comment
- Changes based on review
- Remove llvm:: from the cpp file
- Clang-tidy fix
- Another round of changes based on feedback.