Page MenuHomePhabricator

[FileCheck]] Canonicalize caret location testing

Authored by thopre on Jul 18 2019, 7:32 AM.



Testing of caret location in diagnostic message is currently made with
CHECK directive with the following general format:
CHECK: {{^ \^$}}

James Henderson suggested the following would be more readable:
CHECK: {{^}} ^{{$}}

and when whole lines can be matched (as is the case for command-line
testing where error messages do not include path):
using the option --match-full-lines.

This commit implements these 2 changes on all existing caret position
tests. It also aligns the caret to the character it is trying to match
in the above line.

Diff Detail


Event Timeline

thopre created this revision.Jul 18 2019, 7:32 AM
jhenderson added inline comments.Jul 18 2019, 7:48 AM
8 ↗(On Diff #210561)

In this context, where exactly is the caret actually pointing at? Can spaces be added to one or more lines to get the caret to nicely line up?

9 ↗(On Diff #210561)

Up to you whether you do this, as you may not like it, but I recently realised that you could add leading spaces before the NUMERRCLIFMT: tag to make it line up nicely, whilst still honouring --match-full-lines. Something like:

NUMERRCLIFMT-NEXT:Global define #1: #10VALUE=10 (parsed as: {{\[\[#10
    NUMERRCLIFMT-NEXT:                                             ^
109 ↗(On Diff #210561)

Similar to elsewhere, can you add/remove spaces on one line to make the caret line up with where it should? Same goes with other tests here.

thopre updated this revision to Diff 210587.Jul 18 2019, 8:33 AM
thopre marked 4 inline comments as done.

Address review comment

thopre edited the summary of this revision. (Show Details)Jul 18 2019, 8:33 AM

I've reintroduced the {{$}} in end of lines because it is clear the intent was to test the whole lines in existing tests.

9 ↗(On Diff #210561)

It help show the intent but makes it more difficult to check that the test is correct since the reader has to trust the test writer that the number of space at the beginning of the line equates the number of escape and syntax characters. All in all it's a gain for a loss IMHO so I'm happy to follow your suggestion.

Readability is to some extent in the eye of the reader, so I've restrained myself to one suggestion. Otherwise this review is up to James.

6 ↗(On Diff #210587)

Would this all be easier to read if you added --match-full-lines and then lined up the colons on each directive so the "full line" would start in the same place on each line?
Also then you would not need the leading {{^}} which should help clarify.

thopre marked 2 inline comments as done.Jul 18 2019, 1:32 PM

Readability is to some extent in the eye of the reader, so I've restrained myself to one suggestion. Otherwise this review is up to James.

While I do not like having different alignment for different lines to make the caret match, I do think it makes it obvious the test writer is trying to align the caret to where it should point to. So less readability (but as soon as you have regexes readability is pretty much out of the window) but clear(er) intent. I don't mind either way, except for the patch series to go forward :-)

6 ↗(On Diff #210587)

Unfortunately that is not possible because of the error line above which matches a line that would contain the path to the file and this would not be portable accross system, which is why I restricted this changes to the defines tests which refer to stdin.

It would be nice to have a way to turn that feature on/off for a subset of the directives.

This revision is now accepted and ready to land.Jul 19 2019, 2:30 AM

This is a big improvement to my eyes :)

This revision was automatically updated to reflect the committed changes.
thopre marked an inline comment as done.