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

[River Riddle via Phabricator via llvm-commits]

rriddle 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())
----------------
mehdi_amini wrote:
> 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
This is the style we follow. I generally err on the side of removing "trivial" braces(i.e. 1-2 lines). When they start to span multiple lines it becomes unreadable IMO. @Kayjukh I think your example highlights the style nicely, though in many situations i would also elide the braces around the for.

```
for (...)
  if (...)
     // ...
```


Repository:
  rG LLVM Github Monorepo

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

https://reviews.llvm.org/D80113