This is an archive of the discontinued LLVM Phabricator instance.

[FileCheck, NFC] Split defines.txt in two
ClosedPublic

Authored by thopre on May 8 2019, 6:48 AM.

Event Timeline

thopre created this revision.May 8 2019, 6:48 AM
probinson requested changes to this revision.May 8 2019, 8:54 AM

Aha. The new versions add the word "pattern" to some of the diagnostics, in pattern-defines-diagnostics.txt. That would be the functional change belonging to D60385, which means that test should not pass with an unmodified FileCheck? Please verify.

I am sure this feels like I'm being very picky, but carefully dividing up functional and non-functional reviews makes the reviewer's life much simpler.

llvm/test/FileCheck/pattern-defines-diagnostics.txt
15

"Missing"

This revision now requires changes to proceed.May 8 2019, 8:54 AM
thopre updated this revision to Diff 198683.May 8 2019, 9:21 AM

Fix splitting of NFC part of defines.txt change

thopre added a comment.May 8 2019, 9:28 AM

Aha. The new versions add the word "pattern" to some of the diagnostics, in pattern-defines-diagnostics.txt. That would be the functional change belonging to D60385, which means that test should not pass with an unmodified FileCheck? Please verify.

I am sure this feels like I'm being very picky, but carefully dividing up functional and non-functional reviews makes the reviewer's life much simpler.

My bad, forgot that change. I've merged the two files manually to double check this time and it is now truly NFC and test still passes. Thanks for spotting that.

probinson accepted this revision.May 8 2019, 9:29 AM

Fix the one typo and LGTM.

This revision is now accepted and ready to land.May 8 2019, 9:29 AM
thopre updated this revision to Diff 198685.May 8 2019, 9:29 AM
thopre marked an inline comment as done.

Fix typo in comment

This revision was automatically updated to reflect the committed changes.