[PATCH] D44478: TableGen: Streamline how defs are instantiated

[Artem Belevich via Phabricator via llvm-commits]

tra added a comment.

Just in time. :-) I've ran into the broken foreach-in-multiclass yesterday.



================
Comment at: lib/TableGen/TGParser.cpp:2453
+    CurRec = make_unique<Record>(Records.getNewAnonymousName(), DefLoc, Records,
+                                 true);
+  else
----------------
Nit: `/*Anonymous=*/true`.



================
Comment at: lib/TableGen/TGParser.cpp:2798
   // Keep track of the new generated record definitions.
-  std::vector<Record*> NewRecDefs;
+  std::vector<std::unique_ptr<Record>> NewRecDefs;
 
----------------
Number of items in a multiclass is relatively small. Perhaps we could/should use SmallVector here.


================
Comment at: lib/TableGen/TGParser.cpp:2853-2856
+        // TODO: This MUST happen before template argument resolution. This
+        //       does not make sense and should be changed, but at the time of
+        //       writing, there are existing .td files which rely on this
+        //       implementation detail. It's a bad idea and should be fixed.
----------------
It would be good to have an example or a pointer to something specific so whoever gets to read it later has an ises what needs to be fixed. Perhaps it would make sense to add a disabled test case that  would demonstrate the issue.


================
Comment at: lib/TableGen/TGParser.cpp:2867-2868
+        //
+        // TODO: Whether the name is resolved is basically determined by magic.
+        //       Unfortunately, existing .td files depend on it.
+        R.set(StringInit::get("NAME"), DefmName);
----------------
Same here.


Repository:
  rL LLVM

https://reviews.llvm.org/D44478