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

[Jeff Niu via Phabricator via llvm-commits]

Mogball marked 10 inline comments as done.
Mogball 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
----------------
rriddle wrote:
> I'm getting a slight code smell here that we are slowly building towards a full C++ specification API.
I agree. I'm not necessarily happy about it, but it absolutely is more maintainable than writing to a naked `raw_ostream`.


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


Repository:
  rG LLVM Github Monorepo

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

https://reviews.llvm.org/D113715