[TableGen] [DetailedRecords] Print record name that is null string as ""

[Paul-C-Anagnostopoulos]

Summary

The DetailedRecords backend currently prints a null string record name as nothing, which is confusing. This revision prints the name as "".

|test.td:7|
Defm sequence: |test.td:13|
Superclasses: C
Fields: (none)

versus

""  |test.td:7|
  Defm sequence: |test.td:13|
  Superclasses: C
  Fields: (none)

[dblaikie]

Test coverage?

(& probably use 'empty()' rather than 'size() == 0')

[lattner]

+1 for david's comments, also please fix the clang-format thing of course.

[Paul-C-Anagnostopoulos]

Yes, of course, empty().

Are you asking whether there is a test for this change? There is no test at all for the DetailedRecord backend, just as there is no test for the default print backend. Do you think there should be tests for those two backends?

[dblaikie]

In D95312#2518598, @Paul-C-Anagnostopoulos wrote:

Yes, of course, empty().

Are you asking whether there is a test for this change?

I'm requesting a test be included with/for this change, yeah.

There is no test at all for the DetailedRecord backend, just as there is no test for the default print backend. Do you think there should be tests for those two backends?

Sounds like it - I don't know too much about tablegen, perhaps there's some particular reason not to test these things (perhaps testing them would be difficult to the point of not being worthwhile,etc) - but if there's no particular reason not to, yeah, this should be tested in some way. I don't have strong feelings about what way (API unit test (gtest), lit test, etc) it is tested.

[Paul-C-Anagnostopoulos]

I think the only test that would make sense is a FileCheck test. Of course, that's the only kind of test I've written, so I might be deluding myself.

The problem with a FileCheck test is that it relies on the precise format of the output, which is why the default print records backend of TableGen can never be changed. I'd hate to make DetailedRecords' output rigid like that, because it was written precisely to produce a more detailed dump of the TableGen data in a somewhat friendlier format.

I could write a simple test that just checks that all the global variables, classes, and records are at least included in the output, if not their precise format.

[dblaikie]

In D95312#2518674, @Paul-C-Anagnostopoulos wrote:

I think the only test that would make sense is a FileCheck test. Of course, that's the only kind of test I've written, so I might be deluding myself.

Yep, they tend to be the most convenient & pretty good bang-for-buck, etc.

The problem with a FileCheck test is that it relies on the precise format of the output,

The point of FileCheck is to ensure that the tests are resilient to unrelated format changes (not testing only "is the whole output /exactly/ the same as this example file" (aka: "golden file" tests)) - using regex pattern matches for variable portions, skipping lines unrelated to the particular test/feature, etc.

which is why the default print records backend of TableGen can never be changed.

We change the output of a variety of tools in LLVM pretty deeply when it's warranted - using sed/regular expressions to mass-update tests is a possibility. Perhaps there are features of these TableGen tests and/or the nature of the output that makes the tests harder to maintain/update than others? Not sure.

I'd hate to make DetailedRecords' output rigid like that, because it was written precisely to produce a more detailed dump of the TableGen data in a somewhat friendlier format.

I could write a simple test that just checks that all the global variables, classes, and records are at least included in the output, if not their precise format.

I'll leave any extra testing to account for the overall lack of testing here to you - I'm mostly asking for a test for this patch, not to correct historical choices about understesting the general area.

[Paul-C-Anagnostopoulos]

So what I will do is check that all the global variables, classes, and records are included and have the correct names.

[Paul-C-Anagnostopoulos]

Made change suggested by David and added a test.

[dblaikie]

Looks good - thanks!

llvm/lib/TableGen/DetailedRecordsBackend.cpp

 // Print the records, including the defm sequences, supercasses,
 // and fields.
 void DetailedRecordsEmitter::printRecords(raw_ostream &OS) {
   const auto &RecordList = Records.getDefs();
   printSectionHeading("Records", RecordList.size(), OS);
 
   for (const auto &RecPair : RecordList) {
     auto *const Rec = RecPair.second.get();
-    OS << formatv("\n{0}  |{1}|\n", Rec->getNameInitAsString(),
+    std::string Name = Rec->getNameInitAsString();
+    OS << formatv("\n{0}  |{1}|\n",
+                  (Name.size() == 0) ? "\"\"" : Name,
                   SrcMgr.getFormattedLocationNoOffset(Rec->getLoc().front()));
     printDefms(Rec, OS);
     printSuperclasses(Rec, OS);
     printFields(Rec, OS);
   }
 }
 
 // Print a section heading with the name of the section and