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

[John Demme via Phabricator via llvm-commits]

jdd added inline comments.


================
Comment at: mlir/include/mlir/TableGen/CodeGenHelpers.h:41
+public:
+  inline NamespaceEmitter(raw_ostream &os, const Dialect &dialect) : os(os) {
+    if (!dialect)
----------------
rriddle wrote:
> rriddle wrote:
> > nit: Same comments on inline here and throughout. 
> Seems like this could take the fully qualified cpp namespace string instead of a dialect.
It could, but everything which uses it gets the namespace from the dialect so this enables maximum code sharing.


================
Comment at: mlir/include/mlir/TableGen/TypeDefGenHelpers.h:2
+//
+// Part of the LLVM Project, under the Apache License v2.0 with LLVM Exceptions.
+// See https://llvm.org/LICENSE.txt for license information.
----------------
rriddle wrote:
> Do we need this file for this commit? This looks like it is only used for the testing and I'd rather leave parsing/printer utilities separately.
It's not just for testing. The auto-generated parsers/printers use the templates in this file so whenever one is using auto-generated printers/parsers this file has to be #included. The alternative is to emit this file in the generated code, but I prefer this approach so as to not bloat the emit'ed code.

I get that this template-based design decision is likely to be controversial. This is one piece where I'm looking for high-level feedback.


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

https://reviews.llvm.org/D86904