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

[Nicolai Hähnle via Phabricator via llvm-commits]

nhaehnle added inline comments.


================
Comment at: include/llvm/TableGen/Record.h:1067
+public:
+  using const_iterator = Init *const *;
+
----------------
tra wrote:
> 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.
Makes sense. I believe I was just following some existing pattern elsewhere in the code.


================
Comment at: lib/TableGen/Record.cpp:1379
+VarDefInit *VarDefInit::get(Record *Class, ArrayRef<Init *> Args) {
+  static FoldingSet<VarDefInit> ThePool;
+
----------------
tra wrote:
> Same concern as I've raised in D43680 regarding use of the pool values across multiple invocations of tablegen's main function.
Hmm. I'm less inclined to change this here, since all the other *Init classes work the same way already. I don't see anything that makes VarDefInit qualitatively different from e.g. OpInit.


================
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");
----------------
tra wrote:
> 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".
Good point.


================
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)) {
----------------
tra wrote:
> Nit. `Class->getValue(TArgs[i])` could be extracted into a variable at the beginning of the `for` above.
Done.


Repository:
  rL LLVM

https://reviews.llvm.org/D43756