Page MenuHomePhabricator

Fix a few tests that were missing ':' on CHECK lines and weren't testing anything.

Authored by thakis on Feb 11 2019, 9:11 AM.

Diff Detail

Event Timeline

thakis created this revision.Feb 11 2019, 9:11 AM
rjmccall accepted this revision.Feb 11 2019, 11:45 AM

Good catch! Is there a reasonable way to just make FileCheck itself enforce this, or does that cause too many false positives with tests that are using e.g. x86_64 as the entire prefix?

If we were starting over again, I think ideally the check-prefix would be unconditionally treated as flagging something for FileCheck, so that any sort of unrecognized command after it would produce an error. I doubt there's any real use-case for writing e.g. CHECK in a test case. But that's no longer possible because of the widespread idiom of using things like CHECK-NATIVE as conditionally-enabled prefixes, which is of course ambiguous with typoing a command like CHECK-NEXT or thinking that CHECK-CONT should exist. But it'd be nice to get as close to that as we can without rewriting ten thousand FileCheck tests.

This revision is now accepted and ready to land.Feb 11 2019, 11:45 AM

There has been some progress recently on better FileCheck diagnosis of likely test-writing issues, although to date it mostly requires the human to ask "what is going on here?" explicitly. I can see adding a check to detect the missing-colon-on-otherwise-valid-directive case (including the usual suffixes), and my guess is that isn't too likely to cause excessive churn. The deeper we go into typo-detection the more likely we are to trip over false positives, but this small step seems like a worthy change.
Happy to review such a patch, or somebody can file a PR and CC me.

This revision was automatically updated to reflect the committed changes.
Herald added a project: Restricted Project. · View Herald TranscriptFeb 11 2019, 12:35 PM