[PATCH] D72249: Detemplaize m_Op and RecursivePatternMatcher.

[Alex Zinenko via Phabricator via llvm-commits]

ftynse requested changes to this revision.
ftynse added a comment.
This revision now requires changes to proceed.

Thanks!

Please only change the format of the lines you touch in the commit, e.g. don't run `clang-format -i` but rather `git clang-format` (https://github.com/llvm-mirror/clang/blob/master/tools/clang-format/git-clang-format)



================
Comment at: mlir/include/mlir/IR/Matchers.h:54
+template <typename AttrT>
+struct constant_op_binder {
   AttrT *bind_value;
----------------
This looks like unrelated change, and also wrong format.


================
Comment at: mlir/include/mlir/IR/Matchers.h:129
+  op_matcher() = delete;
+  op_matcher(llvm::StringRef operation) : operationToMatch(operation){};
+  bool match(Operation *op) {
----------------
We don't prefix `StringRef` with `llvm`, it's re-exported into the `mlir` namespace.


================
Comment at: mlir/include/mlir/IR/Matchers.h:129
+  op_matcher() = delete;
+  op_matcher(llvm::StringRef operation) : operationToMatch(operation){};
+  bool match(Operation *op) {
----------------
ftynse wrote:
> We don't prefix `StringRef` with `llvm`, it's re-exported into the `mlir` namespace.
Please add `explicit`


================
Comment at: mlir/include/mlir/IR/Matchers.h:131-133
+    if (operationToMatch == op->getName().getStringRef())
+      return true;
+    return false;
----------------
`return operationToMatch == op->getName().getStringRef()`


================
Comment at: mlir/include/mlir/IR/Matchers.h:137
+private:
+  llvm::StringRef operationToMatch;
 };
----------------
If you intend to store `StringRef`s, please make it clear from the documentation that the caller of `op_matcher` is expected to keep the actual string this refers to alive at least as long as the `op_matcher` instance.


================
Comment at: mlir/include/mlir/IR/Matchers.h:213
   std::tuple<OperandMatchers...> operandMatchers;
+  bool areSame(Operation *op) {
+    if (operationToMatch == op->getName().getStringRef())
----------------
Can you just inline this function into the call place after removing the unneeded `if`?


================
Comment at: mlir/include/mlir/IR/Matchers.h:214-216
+    if (operationToMatch == op->getName().getStringRef())
+      return true;
+    return false;
----------------
Same as above


Repository:
  rG LLVM Github Monorepo

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

https://reviews.llvm.org/D72249