[PATCH] D88832: [TableGen] Add new getAllDerivedDefinitionsTwo function to RecordKeeper

[Paul C. Anagnostopoulos via Phabricator via llvm-commits]

Paul-C-Anagnostopoulos added inline comments.


================
Comment at: llvm/lib/TableGen/Record.cpp:2480-2481
+
+  // First get the class records for the specified classes and make
+  // sure they exist.
+  assert(ClassNames.size() > 0 && "At least one class must be passed.");
----------------
jrtc27 wrote:
> This comment seems a bit pointless
I'm a fan of commenting on the major steps of a function. I was certainly appreciative of them while trying to understand code over the past month.


================
Comment at: llvm/lib/TableGen/Record.cpp:2486
+    if (!Class)
+      PrintFatalError("The class '" + ClassName + "' is not defined\n");
+    ClassRecs.push_back(Class);
----------------
jrtc27 wrote:
> Why change the error message?
I got rid of ERROR: because no other TableGen message includes it. I reworded the message because "Couldn't find ..." strikes me as too informal. I replaced the bang with a newline because that is what most TableGen messages have.


================
Comment at: llvm/utils/TableGen/PseudoLoweringEmitter.cpp:296
 void PseudoLoweringEmitter::run(raw_ostream &o) {
-  Record *ExpansionClass = Records.getClass("PseudoInstExpansion");
-  Record *InstructionClass = Records.getClass("Instruction");
-  assert(ExpansionClass && "PseudoInstExpansion class definition missing!");
-  assert(InstructionClass && "Instruction class definition missing!");
-
-  std::vector<Record*> Insts;
-  for (const auto &D : Records.getDefs()) {
-    if (D.second->isSubClassOf(ExpansionClass) &&
-        D.second->isSubClassOf(InstructionClass))
-      Insts.push_back(D.second.get());
-  }
+  SmallVector<StringRef, 2> Classes = {"PseudoInstExpansion", "Instruction"};
+  std::vector<Record *> Insts =
----------------
jrtc27 wrote:
> StringRef[] should work without needing to mess with classes
Sorry, could you clarify?


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

https://reviews.llvm.org/D88832