[PATCH] D152998: [TableGen][RISCV] Support named arguments

[Michael Maitland via Phabricator via llvm-commits]

michaelmaitland added inline comments.


================
Comment at: llvm/include/llvm/TableGen/Record.h:511
+  Init *getBit(unsigned Bit) const override {
+    return const_cast<ArgumentInit *>(this);
+  }
----------------
Should we call `Value->getBit()` instead? If the argument is a bit, we'd like it to behave like getBit was called on a bit, but if the argument was a IntInit we'd like it to behave like getBit was called on an int.

 Another approach could be to put an `llvm_unreachable`.



================
Comment at: llvm/include/llvm/TableGen/Record.h:513
+  }
+  Init *getCastTo(RecTy *Ty) const override { return nullptr; }
+  Init *convertInitializerTo(RecTy *Ty) const override { return nullptr; }
----------------
Do we really want to return `nullptr` for `getCastTo` and `convertInitializerTo`? Maybe it would be better to `llvm_unreachable("TODO: Implement")`.


================
Comment at: llvm/lib/TableGen/TGParser.cpp:604
+    if (!R.isComplete(UnsolvedArgName))
+      return Error(Loc, "Value not specified for template argument '" +
+                            UnsolvedArgName->getAsUnquotedString() +
----------------
Add test for this error path?


================
Comment at: llvm/lib/TableGen/TGParser.cpp:619
+  if (ArgNames.size() < ArgValues.size())
+    return Error(Loc, "More template args specified than expected");
+
----------------
Add test for error path?


================
Comment at: llvm/lib/TableGen/TGParser.cpp:647
+    if (!Default->isComplete()) {
+      return Error(Loc, "value not specified for template argument (" +
+                            UnsolvedArgName->getAsUnquotedString() +
----------------
Add test for this error path?


================
Comment at: llvm/lib/TableGen/TGParser.cpp:788
 
-  if (ParseTemplateArgValueList(Result.TemplateArgs, CurRec, Result.Rec)) {
+  if (ParseTemplateArgValueList(Result.TemplateArgs, CurRec, Result.Rec,
+                                isDefm)) {
----------------
nit: should we rename `isDefm` to `IsDefm`?


Repository:
  rG LLVM Github Monorepo

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

https://reviews.llvm.org/D152998