[PATCH] D113715: [mlir][ods] AttrOrTypeGen uses Class

[Jeff Niu via Phabricator via llvm-commits]

Mogball added inline comments.


================
Comment at: mlir/include/mlir/TableGen/Class.h:423
+
 /// The OR of two method properties should return method properties. Ensure that
 /// this function is visible to `Class`.
----------------
rriddle wrote:
> LLVM should have functionality for this: https://github.com/llvm/llvm-project/blob/main/llvm/include/llvm/ADT/BitmaskEnum.h
I didn't know about this! But also, it doesn't seem to work because the functions need to be `constexpr` (and there are asserts in them...)


================
Comment at: mlir/tools/mlir-tblgen/AttrOrTypeDefGen.cpp:431
+    ctx.addSubst("_ctxt", "parser.getContext()");
+    parser.indent().getStream().reindent(tgfmt(*parserCode, &ctx).str());
+  }
----------------
rriddle wrote:
> semi unrelated, but the name "reindent" is throwing me off. It doesn't read like something that is emitted to the stream, I keep expecting it to return a reindented string. Can we rename that to something better?
I agree. I initially thought the same.


================
Comment at: mlir/tools/mlir-tblgen/OpDefinitionsGen.cpp:1950-1951
 
-  auto properties = method.isStatic() ? Method::MP_Static : Method::MP_None;
-  if (declaration)
-    properties =
-        static_cast<Method::Property>(properties | Method::MP_Declaration);
-  return opClass.addMethod(method.getReturnType(), method.getName(), properties,
+  auto props = (Method::Static & method.isStatic()) |
+               (Method::Declaration & declaration);
+  return opClass.addMethod(method.getReturnType(), method.getName(), props,
----------------
rriddle wrote:
> Is this right, AFAICT `Method::Declaration & declaration` is always false.
This actually expands to `declaration ? Method::Declaration : Method::None`. If that's not intuitive, I can remove it.


Repository:
  rG LLVM Github Monorepo

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

https://reviews.llvm.org/D113715