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

[Jessica Clarke via Phabricator via llvm-commits]

jrtc27 added inline comments.


================
Comment at: llvm/docs/TableGen/BackGuide.rst:565
 
-* ``getAllDerivedDefinitionsTwo(``\ *classname1*\ ``,`` *classname2*\ ``)`` returns
+* ``getAllDerivedDefinitions(``\ *classvector*\ ``)`` returns
   a vector of ``Record`` references for the concrete records that derive from
----------------
`classnames`


================
Comment at: llvm/lib/TableGen/Record.cpp:2322
 
-std::vector<Record*>
-Record::getValueAsListOfDefs(StringRef FieldName) const {
+std::vector<Record*> Record::getValueAsListOfDefs(StringRef FieldName) const {
   ListInit *List = getValueAsListInit(FieldName);
----------------
Don't reformat this.


================
Comment at: llvm/lib/TableGen/Record.cpp:2472-2473
 
-std::vector<Record *>
-RecordKeeper::getAllDerivedDefinitions(StringRef ClassName) const {
-  Record *Class = getClass(ClassName);
-  if (!Class)
-    PrintFatalError("ERROR: Couldn't find the `" + ClassName + "' class!\n");
-
-  std::vector<Record*> Defs;
-  for (const auto &D : getDefs())
-    if (D.second->isSubClassOf(Class))
-      Defs.push_back(D.second.get());
+/// Get all the concrete records that inherit from all the specified
+/// classes. The classes must be defined.
+std::vector<Record *> RecordKeeper::getAllDerivedDefinitions(
----------------
Don't duplicate what's in the header, it'll only get stale


================
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.");
----------------
This comment seems a bit pointless


================
Comment at: llvm/lib/TableGen/Record.cpp:2486
+    if (!Class)
+      PrintFatalError("The class '" + ClassName + "' is not defined\n");
+    ClassRecs.push_back(Class);
----------------
Why change the error message?


================
Comment at: llvm/lib/TableGen/Record.cpp:2490-2491
+
+  // Now scan all the concrete records and build the vector of records
+  // that inherit from all the classes.
+  for (const auto &OneDef : getDefs()) {
----------------
Also pointless IMO, the code is pretty simple to understand and comments should be for insight into anything of note rather than just repeating what you can easily read for yourself


================
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 =
----------------
StringRef[] should work without needing to mess with classes


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

https://reviews.llvm.org/D88832