[PATCH] D80113: [mlir] Support optional attributes in assembly formats
Jean-Michel Gorius via Phabricator via llvm-commits
llvm-commits at lists.llvm.org
Mon May 18 11:21:25 PDT 2020
Kayjukh 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())
----------------
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 (...)
// ...
}
}
```
Repository:
rG LLVM Github Monorepo
CHANGES SINCE LAST ACTION
https://reviews.llvm.org/D80113/new/
https://reviews.llvm.org/D80113
More information about the llvm-commits
mailing list