This is an archive of the discontinued LLVM Phabricator instance.

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

Authored by Paul-C-Anagnostopoulos on Jan 24 2021, 10:35 AM.

Details

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)

Diff Detail

Event Timeline

Paul-C-Anagnostopoulos requested review of this revision.Jan 24 2021, 10:35 AM
Herald added a project: Restricted Project. · View Herald TranscriptJan 24 2021, 10:35 AM
Paul-C-Anagnostopoulos edited the summary of this revision. (Show Details)Jan 24 2021, 10:36 AM
Paul-C-Anagnostopoulos edited the summary of this revision. (Show Details)

Test coverage?

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

lattner accepted this revision.Jan 24 2021, 2:52 PM

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

This revision is now accepted and ready to land.Jan 24 2021, 2:52 PM

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?

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.

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.

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.

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

Made change suggested by David and added a test.

dblaikie accepted this revision.Jan 26 2021, 2:29 PM

Looks good - thanks!