[PATCH] D73405: [mlir] Add initial support for parsing a declarative operation assembly format
River Riddle via Phabricator via llvm-commits
llvm-commits at lists.llvm.org
Tue Jan 28 10:21:26 PST 2020
rriddle added inline comments.
================
Comment at: mlir/tools/mlir-tblgen/OpFormatGen.cpp:250
+ // Keywords.
+ keyword_start,
+ kw_attr_dict,
----------------
jpienaar wrote:
> Why is keyword spelled out here and in end, but abbreviated elsewhere?
This is used to denote the range of the keywords in the enum, and I don't want to it to seem as if these are 'start' and 'end' keywords.
================
Comment at: mlir/tools/mlir-tblgen/OpFormatGen.cpp:606
+ auto findArg = [&](auto &&range) {
+ auto it = llvm::find_if(range, [=](auto &arg) { return arg.name == name; });
+ return it != range.end() ? &*it : nullptr;
----------------
jpienaar wrote:
> Is 'name' the only value referenced inside lambda?
Yes. I'd rather not add explicit capture to avoid formatting on to a new line. The lambda is simple enough as-is IMO that this isn't necessary.
================
Comment at: mlir/tools/mlir-tblgen/OpFormatGen.cpp:696
+ std::unique_ptr<Element> inputs, results;
+ if (failed(parseToken(Token::l_paren, "expected '(' before argument list")) ||
+ failed(parseTypeDirectiveOperand(inputs)) ||
----------------
jpienaar wrote:
> With these all within one file, having a simple parsing return using bool & using shortcut logic would read easier (E.g., I'm not really convinced LogicalResult helps much here, I feel LogicalResult obscures the parsing logic and doesn't add much value/verification as it is all self-sufficient in this file).
>
Removing the 'failed' wouldn't make it any cleaner/enable formatting. I prefer LogicalResult to enable the use of success/failure, which is where much of the win for this comes in.
Repository:
rG LLVM Github Monorepo
CHANGES SINCE LAST ACTION
https://reviews.llvm.org/D73405/new/
https://reviews.llvm.org/D73405
More information about the llvm-commits
mailing list