[PATCH] D73405: [mlir] Add initial support for parsing a declarative operation assembly format

[River Riddle via Phabricator via llvm-commits]

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