address comments

Author: Moxinilian

mlir/lib/Dialect/IRDL/IR/IRDL.cpp

@@ -94,17 +94,19 @@ LogicalResult OperationOp::verifyRegions() {
     valueNames.emplace_back(kind, std::move(nameSet));
   };
 
-  getBody().walk([&](Operation *op) {
-    TypeSwitch<Operation *>(op)
+  for (Operation &op : getBody().getOps()) {
+    TypeSwitch<Operation *>(&op)
         .Case<OperandsOp>(
             [&](OperandsOp op) { insertNames("operands", op.getNames()); })
         .Case<ResultsOp>(
             [&](ResultsOp op) { insertNames("results", op.getNames()); })
         .Case<RegionsOp>(
             [&](RegionsOp op) { insertNames("regions", op.getNames()); });
-  });
+  }
 
   // Verify that no two operand, result or region share the same name.
+  // The absence of duplicates within each value kind is checked by the
+  // associated operation's verifier.
   for (size_t i : llvm::seq(valueNames.size())) {
     for (size_t j : llvm::seq(i + 1, valueNames.size())) {
       auto [lhs, lhsSet] = valueNames[i];
@@ -131,22 +133,22 @@ static LogicalResult verifyNames(Operation *op, StringRef kindName,
   DenseMap<StringRef, size_t> nameMap;
   for (auto [i, name] : llvm::enumerate(names)) {
     StringRef nameRef = llvm::cast<StringAttr>(name).getValue();
-    if (nameRef.size() == 0)
+    if (nameRef.empty())
       return op->emitOpError()
-             << "name of " << kindName << " number " << i << " is empty";
+             << "name of " << kindName << " #" << i << " is empty";
     if (!llvm::isAlpha(nameRef[0]) && nameRef[0] != '_')
       return op->emitOpError()
-             << "name of " << kindName << " number " << i
+             << "name of " << kindName << " #" << i
              << " must start with either a letter or an underscore";
     if (llvm::any_of(nameRef,
                      [](char c) { return !llvm::isAlnum(c) && c != '_'; }))
       return op->emitOpError()
-             << "name of " << kindName << " number " << i
+             << "name of " << kindName << " #" << i
              << " must contain only letters, digits and underscores";
     if (nameMap.contains(nameRef))
-      return op->emitOpError() << "name of " << kindName << " number " << i
+      return op->emitOpError() << "name of " << kindName << " #" << i
                                << " is a duplicate of the name of " << kindName
-                               << " number " << nameMap[nameRef];
+                               << " #" << nameMap[nameRef];
     nameMap.insert({nameRef, i});
   }
 

mlir/test/Dialect/IRDL/invalid.irdl.mlir

@@ -25,7 +25,7 @@ irdl.dialect @testd {
 irdl.dialect @testd {
   irdl.type @type {
     %0 = irdl.any
-    // expected-error@+1 {{name of parameter number 0 must contain only letters, digits and underscores}}
+    // expected-error@+1 {{name of parameter #0 must contain only letters, digits and underscores}}
     irdl.parameters(test$test: %0)
   }
 }
@@ -35,7 +35,7 @@ irdl.dialect @testd {
 irdl.dialect @testd {
   irdl.operation @op {
     %0 = irdl.any
-    // expected-error@+1 {{name of result number 0 must contain only letters, digits and underscores}}
+    // expected-error@+1 {{name of result #0 must contain only letters, digits and underscores}}
     irdl.results(test$test: %0)
   }
 }
@@ -45,7 +45,7 @@ irdl.dialect @testd {
 irdl.dialect @testd {
   irdl.operation @op {
     %0 = irdl.any
-    // expected-error@+1 {{name of operand number 0 must contain only letters, digits and underscores}}
+    // expected-error@+1 {{name of operand #0 must contain only letters, digits and underscores}}
     irdl.operands(test$test: %0)
   }
 }
@@ -55,7 +55,7 @@ irdl.dialect @testd {
 irdl.dialect @testd {
   irdl.type @type {
     %0 = irdl.any
-    // expected-error@+1 {{name of parameter number 2 is a duplicate of the name of parameter number 0}}
+    // expected-error@+1 {{name of parameter #2 is a duplicate of the name of parameter #0}}
     irdl.parameters(foo: %0, bar: %0, foo: %0)
   }
 }
@@ -65,7 +65,7 @@ irdl.dialect @testd {
 irdl.dialect @testd {
   irdl.operation @op {
     %0 = irdl.any
-    // expected-error@+1 {{name of result number 2 is a duplicate of the name of result number 0}}
+    // expected-error@+1 {{name of result #2 is a duplicate of the name of result #0}}
     irdl.results(foo: %0, bar: %0, foo: %0)
   }
 }
@@ -75,7 +75,7 @@ irdl.dialect @testd {
 irdl.dialect @testd {
   irdl.operation @op {
     %0 = irdl.any
-    // expected-error@+1 {{name of operand number 2 is a duplicate of the name of operand number 0}}
+    // expected-error@+1 {{name of operand #2 is a duplicate of the name of operand #0}}
     irdl.operands(foo: %0, bar: %0, foo: %0)
   }
 }

mlir/test/Dialect/IRDL/regions-ops.irdl.mlir

@@ -22,7 +22,7 @@ irdl.dialect @testRegionsOpMissingName {
 irdl.dialect @testRegionsOpWrongName {
     irdl.operation @op {
         %r1 = irdl.region
-        // expected-error @below {{name of region number 0 must contain only letters, digits and underscores}}
+        // expected-error @below {{name of region #0 must contain only letters, digits and underscores}}
         irdl.regions(test$test: %r1)
     }
 }
@@ -32,7 +32,7 @@ irdl.dialect @testRegionsOpWrongName {
 irdl.dialect @testRegionsDuplicateName {
     irdl.operation @op {
         %r1 = irdl.region
-        // expected-error @below {{name of region number 2 is a duplicate of the name of region number 0}}
+        // expected-error @below {{name of region #2 is a duplicate of the name of region #0}}
         irdl.regions(foo: %r1, bar: %r1, foo: %r1)
     }
 }