[PATCH] D86904: [mlir] Support for defining Types in tblgen

[John Demme via Phabricator via llvm-commits]

jdd marked an inline comment as done.
jdd added inline comments.


================
Comment at: mlir/test/lib/Dialect/Test/TestTypes.cpp:63
+  SmallVector<int, 4> arrayOfInts;
+  if (parser.parseLess())
+    return Type();
----------------
rriddle wrote:
> nit: Merge all of these together using `||`
Nice!


================
Comment at: mlir/test/lib/Dialect/Test/TestTypes.cpp:93
+  auto intArray = getArrayOfInts();
+  for (size_t idx = 0; idx < intArray.size(); idx++) {
+    printer << intArray[idx];
----------------
rriddle wrote:
> nit: Use llvm::interleaveComma here instead.
That's handy


================
Comment at: mlir/test/lib/Dialect/Test/TestTypes.cpp:101
+
+bool operator==(const FieldInfo &a, const FieldInfo &b) {
+  return a.name == b.name && a.type == b.type;
----------------
rriddle wrote:
> The definitions of methods in header files should be fully qualified `::` and not placed in a namespace in the .cpp.
I've removed two from the header


================
Comment at: mlir/tools/mlir-tblgen/TypeDefGen.cpp:222
+    // well known print/parse dispatch function declarations
+    os << "  ::mlir::Type generatedTypeParser(::mlir::MLIRContext* ctxt, "
+          "::mlir::DialectAsmParser& parser, ::llvm::StringRef mnenomic);\n";
----------------
rriddle wrote:
> Seems like this would be something only generated in the source file and marked as static.
The dialect calls these from its parseType and printType functions so we must declare them here.


Repository:
  rG LLVM Github Monorepo

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

https://reviews.llvm.org/D86904