[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:412
+  /// Add a template parameter.
+  void addTemplateParam(std::string param) {
+    templateParams.insert(std::move(param));
----------------
rriddle wrote:
> 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.
I went for the stringify route because occasionally in ODS we create std::string and maybe something takes ownership of it. I got tired of debugging segmentation faults because `.str()` wasn't being called somewhere and a StringRef or Twine or format object was outliving its value.


Repository:
  rG LLVM Github Monorepo

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

https://reviews.llvm.org/D113715