This is an archive of the discontinued LLVM Phabricator instance.

[NFC] Unforget a colon in a few CHECK: directives.
ClosedPublic

Authored by NoQ on Jul 10 2019, 11:49 AM.

Details

Summary

I noticed that people occasionally forget that unlike CHECK:, directives CHECK and CHECK : are not reacted upon by FileCheck. Tests still pass, but they don't actually test anything.

So i grepped real quick and found these four missing colons, one of which (the one in Analysis/cfg-rich-constructors.cpp) is mine. All four are in clang; there aren't any missing colons in other projects in the monorepo.

I'm not entirely sure about the change OpenMP/sections_lastprivate_codegen.cpp: it seems that replacing CHECK: with CHECK is a common way of making FIXME tests, but there's no indication anywhere that this is a known issue. The test was added in D11619.

Diff Detail

Repository
rL LLVM

Event Timeline

NoQ created this revision.Jul 10 2019, 11:49 AM
ABataev added inline comments.Jul 10 2019, 11:51 AM
clang/test/OpenMP/sections_lastprivate_codegen.cpp
31 ↗(On Diff #209037)

I'll fix this test.

NoQ updated this revision to Diff 209046.Jul 10 2019, 12:35 PM
NoQ added a reviewer: Anastasia.

Found one more forgotten colon by grepping for //CHECK instead of // CHECK .

Removed the FIXME for @ABataev so that not to cause merge conflicts when he fixes the test.

P.S. I didn't grep for custom FileCheck prefixes. Those may have more bugs.

NoQ marked an inline comment as done.Jul 10 2019, 12:36 PM
NoQ added inline comments.
clang/test/OpenMP/sections_lastprivate_codegen.cpp
31 ↗(On Diff #209037)

Yay!

thakis added a subscriber: thakis.Jul 10 2019, 1:03 PM

(previously: D58061)

NoQ updated this revision to Diff 209060.Jul 10 2019, 1:50 PM
NoQ added reviewers: davidxl, aprantl, JDevlieghere.
NoQ set the repository for this revision to rL LLVM.
NoQ added a project: Restricted Project.

Add three more fixes - two with CHECK-NEXT and one with CHECK-NOT, in LLVM and compiler-rt.

One of the LLVM changes doesn't pass after the fix, so i added a FIXME instead.

I didn't immediately figure out how to compile compiler-rt so i didn't really have a look if the test passes; could use some help or i guess the buildbots will eventually tell me :)

NoQ added a comment.Jul 10 2019, 1:51 PM

(previously: D58061)

Aha, that explains why most of these are new :)

NoQ added a comment.Jul 10 2019, 1:52 PM

Yeah, and i removed the one in addrspace-operators.cl because it has already been fixed in rC365666.

The dsymutil part LGTM

I didn't know spaces are not allowed between "CHECK" and ":". CodeGen tests LGTM.

NoQ added a comment.Jul 11 2019, 7:15 PM

Thanks all! I guess i'll commit.

This revision was not accepted when it landed; it landed in state Needs Review.Jul 11 2019, 7:18 PM
This revision was automatically updated to reflect the committed changes.