[PATCH] D43756: TableGen: Delay instantiating inline anonymous records

[Artem Belevich via Phabricator via llvm-commits]

tra added a comment.

Yay! Another of my favorite complaints about tablegen will be gone! Thank you!
Looks good overall,  except for that concern about static pool and using tablegen as a library.



================
Comment at: include/llvm/TableGen/Record.h:1067
+public:
+  using const_iterator = Init *const *;
+
----------------
Nit. I'd move it down to args_begin/args_end so it's easy to see the details of const_iterator next to the functions that return it. It's fine either way.


================
Comment at: lib/TableGen/Record.cpp:1379
+VarDefInit *VarDefInit::get(Record *Class, ArrayRef<Init *> Args) {
+  static FoldingSet<VarDefInit> ThePool;
+
----------------
Same concern as I've raised in D43680 regarding use of the pool values across multiple invocations of tablegen's main function.


================
Comment at: lib/TableGen/TGParser.cpp:1365-1368
+    ArrayRef<Init *> TArgs = Class->getTemplateArgs();
+    if (TArgs.size() < Args.size()) {
+      Error(NameLoc,
+            "More template args specified than expected");
----------------
TArgs could use a better name. Something along the lines of `ExpectedArgNum`.  Calling it TArgs makes the error message somewhat confusing. My first thought was "Hold on a sec. We've just checked that template args (TArgs) is *less* than the number of [expected] arguments (Args)... Oh! TArgs is not the template args, it is the expected args".


================
Comment at: lib/TableGen/TGParser.cpp:1375
+        if (TypedInit *TI = dyn_cast<TypedInit>(Args[i])) {
+          RecTy *ExpectedType = Class->getValue(TArgs[i])->getType();
+          if (!TI->getType()->typeIsConvertibleTo(ExpectedType)) {
----------------
Nit. `Class->getValue(TArgs[i])` could be extracted into a variable at the beginning of the `for` above.


Repository:
  rL LLVM

https://reviews.llvm.org/D43756