[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:219-226
+  /// Kinds for LLVM-style RTTI.
+  enum Kind {
+    Method,
+    UsingDeclaration,
+    VisibilityDeclaration,
+    Field,
+    ExtraClassDeclaration
----------------
I'm getting a slight code smell here that we are slowly building towards a full C++ specification API.


================
Comment at: mlir/include/mlir/TableGen/Class.h:412
+  /// Add a template parameter.
+  void addTemplateParam(std::string param) {
+    templateParams.insert(std::move(param));
----------------
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.


================
Comment at: mlir/include/mlir/TableGen/Class.h:451-452
+
+/// This class describes a class field. Class fields are always private and are
+/// always declared at the bottom of the class declaration.
+class Field : public ClassDeclarationBase<ClassDeclaration::Field> {
----------------
Not sure this comment is true given that we have isPublic below.


================
Comment at: mlir/include/mlir/TableGen/Class.h:460
+  void writeDeclTo(raw_indented_ostream &os) const override;
+  bool isPublic() const { return publicField; }
+
----------------
Missing comment here?


================
Comment at: mlir/include/mlir/TableGen/Class.h:465
+  std::string name;
+  const bool publicField;
+};
----------------
nit: Drop the const, we don't generally add them.

Also please document these fields.


================
Comment at: mlir/include/mlir/TableGen/Class.h:610
 
+  virtual void finalize();
+
----------------
This needs some documentation.


================
Comment at: mlir/include/mlir/TableGen/Format.h:54
+  // Create a format context with a list of substitutions.
+  FmtContext(std::initializer_list<std::pair<StringRef, StringRef>> subs);
+
----------------
nit: I'd use ArrayRef here instead.


================
Comment at: mlir/lib/TableGen/AttrOrTypeDef.cpp:110-117
+SmallVector<AttrOrTypeParameter> AttrOrTypeDef::getParameters() const {
+  SmallVector<AttrOrTypeParameter> parameters;
   if (auto *parametersDag = def->getValueAsDag("parameters")) {
     for (unsigned i = 0, e = parametersDag->getNumArgs(); i < e; ++i)
       parameters.push_back(AttrOrTypeParameter(parametersDag, i));
   }
+  return parameters;
----------------
Can we just put the vector on the class and return ArrayRef instead?


================
Comment at: mlir/lib/TableGen/Class.cpp:301-310
+  SmallVector<std::unique_ptr<Method>> publicMethods, privateMethods;
+  for (auto &method : methods) {
+    (method->isPrivate() ? privateMethods : publicMethods)
+        .push_back(std::move(method));
   }
+  methods.clear();
+
----------------
This needs some documentation.


================
Comment at: mlir/tools/mlir-tblgen/AttrOrTypeDefGen.cpp:161
+  /// An optional attribute or type storage class. The storage class will
+  /// existif and only if the def has more than zero parameters.
+  Optional<Class> storageCls;
----------------



Repository:
  rG LLVM Github Monorepo

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

https://reviews.llvm.org/D113715