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

[Jessica Clarke via Phabricator via llvm-commits]

jrtc27 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.");
----------------
Paul-C-Anagnostopoulos wrote:
> 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.
It's a loop to call getClass and add the result to a vector. If we commented everything like that in LLVM it would be a complete nightmare.


================
Comment at: llvm/lib/TableGen/Record.cpp:2486
+    if (!Class)
+      PrintFatalError("The class '" + ClassName + "' is not defined\n");
+    ClassRecs.push_back(Class);
----------------
Paul-C-Anagnostopoulos wrote:
> 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.
There are a few others like that in llvm/utils/TableGen (and there was already a newline)


================
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 =
----------------
Paul-C-Anagnostopoulos wrote:
> jrtc27 wrote:
> > StringRef[] should work without needing to mess with classes
> Sorry, could you clarify?
```StringRef Classes[] = ...```


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

https://reviews.llvm.org/D88832