[PATCH] D80113: [mlir] Support optional attributes in assembly formats

[Mehdi AMINI via Phabricator via llvm-commits]

mehdi_amini added inline comments.


================
Comment at: mlir/tools/mlir-tblgen/OpFormatGen.cpp:893
+    // Collect the optional attributes.
+    if (auto *opt = dyn_cast<OptionalElement>(it.get()))
+      for (auto &elem : opt->getElements())
----------------
Kayjukh wrote:
> ftynse wrote:
> > Kayjukh wrote:
> > > ftynse wrote:
> > > > Kayjukh wrote:
> > > > > nit: It could be advisable to use some curly braces for such nested statements.
> > > > MLIR is actually quite adamant on the opposite
> > > Interesting. Is there a mention of this in the LLVM coding standards? If not I guess it is mostly up to preference.
> > It's not in LLVM standards or MLIR fixups to them. In practice, MLIR is aggressive on eliding the braces for single-line one-statement bodies.
> Sure, for single-line bodies I totally agree that avoiding the braces can make things more readable. What I was pointing out in the first comment was
> ```
> if (...)
>   for (...)
>     if (...)
>       // ...
> ```
> which I find less readable and potentially more error-prone than
> ```
> if (...) {
>   for (...) {
>     if (...)
>       // ...
>   }
> }
> ```
If there is a single statement body inside the `if`, we'll go without braces in MLIR in general I believe


Repository:
  rG LLVM Github Monorepo

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

https://reviews.llvm.org/D80113