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

[River Riddle via Phabricator via llvm-commits]

rriddle 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`.
----------------
LLVM should have functionality for this: https://github.com/llvm/llvm-project/blob/main/llvm/include/llvm/ADT/BitmaskEnum.h


================
Comment at: mlir/include/mlir/TableGen/Class.h:412
+  /// Add a template parameter.
+  void addTemplateParam(std::string param) {
+    templateParams.insert(std::move(param));
----------------
Mogball wrote:
> rriddle wrote:
> > Prefer accepting `const Twine  &` instead of std::string. It accepts many more parameter types (e.g. StringRef, std::string, llvm::formatv, etc.), and can easily be converted to string.
> Good point, but what's your opinion on the `stringify` function I added? I wanted to elide copying `std::string` when it's passed in directly, and I've had some issues with `const Twine &`; for example,
> 
> ```
> void one(const Twine &value = "");
> void one(bool value = false);
> ```
> 
> Confusingly, `one("C string")` calls `one(bool)`
Which methods do we have that takes bool or Twine? If it isn't a lot of methods, we can always just add an overload specialized for `const char *`/`StringRef`.

> but what's your opinion on the stringify function I added?
It's giving me an off feeling, but that may be that we generally try not to use std::string directly.


================
Comment at: mlir/lib/TableGen/Class.cpp:203-205
+  default:
+    assert(visibility == Visibility::Private);
+    return os << "private";
----------------
AFAIR we rely on the compiler to warn/error about missing cases.


================
Comment at: mlir/lib/TableGen/Class.cpp:297-298
+  for (auto &method : methods) {
+    (method->isPrivate() ? privateMethods : publicMethods)
+        .push_back(std::move(method));
+  }
----------------
Please switch this to an if/else to make it easier to read.


================
Comment at: mlir/lib/TableGen/Class.cpp:309
+    declare<VisibilityDeclaration>(Visibility::Public);
+  llvm::for_each(publicMethods, declareMethod);
+
----------------
Please use proper foreach loops for these, the llvm::for_each doesn't add much.


================
Comment at: mlir/tools/mlir-tblgen/AttrOrTypeDefGen.cpp:211-216
+  llvm::for_each(def.getTraits(), [&](auto &trait) {
+    defParent.addTemplateParam(
+        isa<NativeTrait>(&trait)
+            ? cast<NativeTrait>(&trait)->getFullyQualifiedTraitName()
+            : cast<InterfaceTrait>(&trait)->getFullyQualifiedTraitName());
+  });
----------------
I'd just use a proper foreach loop here, using llvm::for_each doesn't really add much.


================
Comment at: mlir/tools/mlir-tblgen/AttrOrTypeDefGen.cpp:431
+    ctx.addSubst("_ctxt", "parser.getContext()");
+    parser.indent().getStream().reindent(tgfmt(*parserCode, &ctx).str());
+  }
----------------
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?


================
Comment at: mlir/tools/mlir-tblgen/AttrOrTypeDefGen.cpp:462-463
+  // All interface methods are declaration-only.
+  auto props = Method::Declaration |
+               (method.isStatic() ? Method::Static : Method::Const);
+  SmallVector<MethodParameter> params;
----------------
?


================
Comment at: mlir/tools/mlir-tblgen/AttrOrTypeDefGen.cpp:511-515
+  std::string keyType("std::tuple<");
+  llvm::interleave(
+      params, [&](auto &param) { keyType += param.getCppType(); },
+      [&] { keyType += ", "; });
+  keyType.push_back('>');
----------------
raw_string_ostream would likely be cleaner here.


================
Comment at: mlir/tools/mlir-tblgen/AttrOrTypeDefGen.cpp:626
   /// The prefix of the tablegen def name, e.g. Attr or Type.
-  StringRef defTypePrefix;
+  const StringRef defType;
   /// The C++ base value type of the def, e.g. Attribute or Type.
----------------
Drop the consts, we don't really add them unless really necessary (i.e. if they add value).


================
Comment at: mlir/tools/mlir-tblgen/OpClass.h:25
+
+  void finalize() override;
+
----------------
Can you document what this does? The doc on the other finalize was quite nice.


================
Comment at: mlir/tools/mlir-tblgen/OpDefinitionsGen.cpp:754
   auto emitAttrWithStorageType = [&](StringRef name, Attribute attr) {
-    auto *method =
-        opClass.addMethod(attr.getStorageType(), (name + "Attr").str());
+    auto *method = opClass.addMethod(attr.getStorageType(), (name + "Attr"));
     if (!method)
----------------
Same comment about () here and elsewhere.


================
Comment at: mlir/tools/mlir-tblgen/OpDefinitionsGen.cpp:852
     auto *method =
-        opClass.addMethod("void", (setterName + "Attr").str(),
+        opClass.addMethod("void", (setterName + "Attr"),
                           MethodParameter(attr.getStorageType(), "attr"));
----------------
Can we drop the ()?


================
Comment at: mlir/tools/mlir-tblgen/OpDefinitionsGen.cpp:876
     auto *method = opClass.addMethod(
-        "::mlir::Attribute", ("remove" + upperInitial + suffix + "Attr").str());
+        "::mlir::Attribute", ("remove" + upperInitial + suffix + "Attr"));
     if (!method)
----------------
Can we drop the ()? We generally don't wrap unless necessary.


================
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,
----------------
Is this right, AFAICT `Method::Declaration & declaration` is always false.


Repository:
  rG LLVM Github Monorepo

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

https://reviews.llvm.org/D113715