[PATCH] D74143: [MLIR] Adding attribute setters generation for table gen

[Alexandre Eichenberger via Phabricator via llvm-commits]

AlexEichenberger marked 2 inline comments as done.
AlexEichenberger added a comment.

tx for the valuable input



================
Comment at: mlir/tools/mlir-tblgen/OpDefinitionsGen.cpp:393
+  FmtContext fctx;
+  fctx.withBuilder("mlir::Builder(this->getContext())");
+  auto emitAttrWithStorageType = [&](StringRef name, Attribute attr) {
----------------
jpienaar wrote:
> AlexEichenberger wrote:
> > rriddle wrote:
> > > What is the context used for?
> > It is used in the same context as the getters. In our case, we use this functionality in a shape inference pass. Is it at risk to vanish before a subsequent pass?
> To rephrase the question: fctx does not seemed to be used anywhere here and so could be removed.
good catch, sorry I missed the meaning of context here 


================
Comment at: mlir/tools/mlir-tblgen/OpDefinitionsGen.cpp:388-390
+// Generate raw named setter type. This is a wrapper class that allows
+// setting to the attributes via setters instead of having to use
+// the string interface for better compile time verification.
----------------
jpienaar wrote:
> Reflow?
> 
>   // Generate raw named setter type. This is a wrapper class that allows setting
>   // the attributes via setters instead of having to use the string interface
>   // for better compile time verification.
> 
> This comment may also be more relevant inside: e.g., there are potentially multiple setters, as with getters, and this is one specific to raw setters.
performed reflow and moved comment to requested spot


Repository:
  rG LLVM Github Monorepo

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

https://reviews.llvm.org/D74143