[PATCH] D96987: [flang][fir][NFC] Move remaining types to TableGen type definition

[River Riddle via Phabricator via llvm-commits]

rriddle added a comment.

Do the parsers/printers here have any test coverage?



================
Comment at: flang/include/flang/Optimizer/Dialect/FIRTypes.td:86
+        "mlir::Type":$eleTy,
+        "mlir::AffineMapAttr":$map), [{
+      return Base::get(eleTy.getContext(), eleTy, map);
----------------
You can use CArg to provided a default value for the `map` param.


================
Comment at: flang/include/flang/Optimizer/Dialect/FIRTypes.td:425
+    mlir::Type getElementType() const {
+      return getEleTy();
+    }
----------------
Renaming the parameter to `elementType` would remove the need for this.


================
Comment at: flang/lib/Optimizer/Dialect/FIRType.cpp:300
+  return eleTy.isa<BoxType>() || eleTy.isa<BoxCharType>() ||
+         eleTy.isa<BoxProcType>() || eleTy.isa<ShapeType>() ||
+         eleTy.isa<ShapeShiftType>() || eleTy.isa<SliceType>() ||
----------------
You should be able to do: `isa<BoxType, BoxCharType, ...>()`


================
Comment at: flang/lib/Optimizer/Dialect/FIRType.cpp:328
+  if (parser.parseGreater()) {
+    parser.emitError(parser.getCurrentLocation(), "expected '>'");
+    return {};
----------------
This error message should be redundant, an error is already emitted for most of these cases. This seems to point to a lack of test coverage. We also try to avoid double error messages in general.


================
Comment at: flang/lib/Optimizer/Dialect/FIRType.cpp:338
+    printer << ", ";
+    printer.printAttribute(map);
+  }
----------------
nit: You should be able to just stream this in: `printer << ", " << map;`


================
Comment at: flang/lib/Optimizer/Dialect/FIRType.cpp:768
   auto shape = getShape();
   for (unsigned i{rows}, size{dim}; i < size; ++i)
     if (shape[i] != getUnknownExtent())
----------------
`i = rows`? The initialization here looks really weird,


Repository:
  rG LLVM Github Monorepo

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

https://reviews.llvm.org/D96987