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

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

Paul-C-Anagnostopoulos marked 3 inline comments as done.
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:
> 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.
Okay, I'll remove them. The code with more comments has been easier for me to understand.


================
Comment at: llvm/lib/TableGen/Record.cpp:2486
+    if (!Class)
+      PrintFatalError("The class '" + ClassName + "' is not defined\n");
+    ClassRecs.push_back(Class);
----------------
jrtc27 wrote:
> 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)
Do you want me to restore the original comment?


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

https://reviews.llvm.org/D88832