This is an archive of the discontinued LLVM Phabricator instance.

[FileCheck]Remove assertions that prevent matching an empty string at file start before CHECK-NEXT/SAME
ClosedPublic

Authored by jhenderson on Feb 28 2019, 6:31 AM.

Details

Summary

This patch removes two assertions that were preventing me writing a test that checked an empty line followed by some text. Here is an example of a set of patterns that caused FileCheck to assert.

CHECK: {{^$}}
CHECK-NEXT: foo()

The assertion was because the current location the CHECK-NEXT was scanning from was the start of the buffer. A similar issue occurred with CHECK-SAME. These assertions don't protect against anything, as there is already an error check that checks that CHECK-NEXT/EMPTY/SAME don't appear first in the checks, and the following code works fine if the pointer is at the start of the input.

Diff Detail

Repository
rL LLVM

Event Timeline

jhenderson created this revision.Feb 28 2019, 6:31 AM
Herald added a project: Restricted Project. · View Herald TranscriptFeb 28 2019, 6:31 AM

I think the asserts should stay, they ensure that the call to CountNumNewlinesBetween is counting the newline in the right segment of text. I'd rather add an extra condition to the assert "or it's first NEXT". In any case I do not understand why the asserts search from the start of the buffer given that there is a CHECK before the CHECK-SAME and CHECK-NEXT in your examples. Could you detail a bit?

I think the asserts should stay, they ensure that the call to CountNumNewlinesBetween is counting the newline in the right segment of text.

I'm not sure I follow how the asserts ensure this? All it shows is that the current buffer pointer is not the same as the start of the memory block it is in, i.e. that it has moved forwards.

In any case I do not understand why the asserts search from the start of the buffer given that there is a CHECK before the CHECK-SAME and CHECK-NEXT in your examples. Could you detail a bit?

The Buffer contains the remainder of the text in the input following the previous match, or the entire text if it is the first match. For example, if the input string was abc and there was a CHECK: a first, Buffer contains bc, wheras CHECK: b would result in the Buffer just containing c. The problem is that the pattern CHECK: {{^$}} matches exactly 0 characters, thus the Buffer is the same before and after the match, and therefore the Buffer is still pointing at the start of the memory block.

thopre accepted this revision.Feb 28 2019, 7:28 AM

I think the asserts should stay, they ensure that the call to CountNumNewlinesBetween is counting the newline in the right segment of text.

I'm not sure I follow how the asserts ensure this? All it shows is that the current buffer pointer is not the same as the start of the memory block it is in, i.e. that it has moved forwards.

In any case I do not understand why the asserts search from the start of the buffer given that there is a CHECK before the CHECK-SAME and CHECK-NEXT in your examples. Could you detail a bit?

The Buffer contains the remainder of the text in the input following the previous match, or the entire text if it is the first match. For example, if the input string was abc and there was a CHECK: a first, Buffer contains bc, wheras CHECK: b would result in the Buffer just containing c. The problem is that the pattern CHECK: {{^$}} matches exactly 0 characters, thus the Buffer is the same before and after the match, and therefore the Buffer is still pointing at the start of the memory block.

Oh right, completely misread it. That is much clearer, thank you. Yes LGTM!

This revision is now accepted and ready to land.Feb 28 2019, 7:28 AM

So we didn't have any tests that verified the assertion? That was an oversight.
FTR, the CHECK: {{^$}} is ensuring the first line is empty? I see that CHECK-EMPTY isn't allowed as the first directive, unfortunately.

FTR, the CHECK: {{^$}} is ensuring the first line is empty? I see that CHECK-EMPTY isn't allowed as the first directive, unfortunately.

This directive looks for the next empty line, even if that's not the first line. Interestingly, when it's not the first directive, its search range starts after the previous match, so if the remainder of the previous match's line is empty (even if the full line is not empty), it'll match there.

The following appears to be equivalent to what an initial CHECK-EMPTY should do:

CHECK: {{^}}
CHECK-SAME: {{^$}}

Looking at the test suite, we also have what appears to be an equivalent to what an initial CHECK-NEXT should do (the "next" line is the first line in that case):

CHECK: {{^}}
CHECK-SAME: some text

Maybe we should just permit initial CHECK-NEXT and CHECK-EMPTY rather requiring people to use these hacks.

(Putting them after a CHECK-DAG is probably a mistake though. Also, an initial CHECK-SAME has no clear meaning to me.)

FTR, the CHECK: {{^$}} is ensuring the first line is empty? I see that CHECK-EMPTY isn't allowed as the first directive, unfortunately.

This directive looks for the next empty line, even if that's not the first line. Interestingly, when it's not the first directive, its search range starts after the previous match, so if the remainder of the previous match's line is empty (even if the full line is not empty), it'll match there.

Ah right.

The following appears to be equivalent to what an initial CHECK-EMPTY should do:

CHECK: {{^}}
CHECK-SAME: {{^$}}

Yep. So if we allow an initial CHECK-SAME, defining the search range as starting at the beginning of the input, then an initial CHECK-SAME: stuff verifies that stuff appears on the first line, and CHECK-SAME: {{^$}} verifies the first line is empty. That seems pretty intuitive to me.

Looking at the test suite, we also have what appears to be an equivalent to what an initial CHECK-NEXT should do (the "next" line is the first line in that case):

CHECK: {{^}}
CHECK-SAME: some text

Maybe we should just permit initial CHECK-NEXT and CHECK-EMPTY rather requiring people to use these hacks.

SGTM.

Am I reading the docs rightly, it looks like CHECK-EMPTY is exactly CHECK-NEXT: {{^$}} isn't it? Did we have some other rationale for adding EMPTY, which now escapes me?

(Putting them after a CHECK-DAG is probably a mistake though.

Yep.

Also, an initial CHECK-SAME has no clear meaning to me.)

See above.

The following appears to be equivalent to what an initial CHECK-EMPTY should do:

CHECK: {{^}}
CHECK-SAME: {{^$}}

Yep. So if we allow an initial CHECK-SAME, defining the search range as starting at the beginning of the input, then an initial CHECK-SAME: stuff verifies that stuff appears on the first line, and CHECK-SAME: {{^$}} verifies the first line is empty. That seems pretty intuitive to me.

I see what you mean, but see below.

Am I reading the docs rightly, it looks like CHECK-EMPTY is exactly CHECK-NEXT: {{^$}} isn't it? Did we have some other rationale for adding EMPTY, which now escapes me?

They're not the same. If the previous match occurs at the end of the line, your CHECK-NEXT will match at the end of that line and then complain it's not on the next line. CHECK-EMPTY skips a newline before looking for an empty line.

Also, an initial CHECK-SAME has no clear meaning to me.)

See above.

Here's how I see it, but I realize it's subjective. If we say that an initial CHECK-SAME matches the first line, then, to be consistent with the relationship CHECK-SAME and CHECK-NEXT have everywhere else, an initial CHECK-NEXT should skip a new line and thus match the second line. That means that, in order to match consecutive lines starting with the first line, we have:

CHECK-SAME: first line
CHECK-NEXT: second line
CHECK-NEXT: third line

But the following seems more intuitive to me:

CHECK-NEXT: first line
CHECK-NEXT: second line
CHECK-NEXT: third line

To say it more abstractly:

  • CHECK-NEXT means match on the next line after the previous match.
  • CHECK-SAME means match on the same line as the previous match.

If there is no previous match because this is the first directive, then that translates to:

  • CHECK-NEXT means match on the next line after no match yet, and that intuitively sounds like the first line to me.
  • CHECK-SAME means match on the same line as no match yet, but that sounds impossible to me.

Here's how I see it, but I realize it's subjective. If we say that an initial CHECK-SAME matches the first line, then, to be consistent with the relationship CHECK-SAME and CHECK-NEXT have everywhere else, an initial CHECK-NEXT should skip a new line and thus match the second line. That means that, in order to match consecutive lines starting with the first line, we have:

CHECK-SAME: first line
CHECK-NEXT: second line
CHECK-NEXT: third line

That matches my intuition.

But the following seems more intuitive to me:

CHECK-NEXT: first line
CHECK-NEXT: second line
CHECK-NEXT: third line

To say it more abstractly:

  • CHECK-NEXT means match on the next line after the previous match.
  • CHECK-SAME means match on the same line as the previous match.

If there is no previous match because this is the first directive, then that translates to:

  • CHECK-NEXT means match on the next line after no match yet, and that intuitively sounds like the first line to me.
  • CHECK-SAME means match on the same line as no match yet, but that sounds impossible to me.

I would say it this way:

  • CHECK-NEXT means match on the next line after the starting point
  • CHECK-SAME means match on the same line as the starting point

which is more in line with the search-range model.

thopre added a comment.Mar 1 2019, 1:26 AM

If we are talking about matching a specific line (in this case the first line), I wonder if we could not extend the syntax of directives to accept a line number, e.g. CHECK-EMPTY-L1. Or if we think only the first line deserve to be checked explicitely simply a CHECK-FIRST. The fact that we are arguing about what would the meaning of CHECK-SAME and CHECK-NEXT on the first line be suggests that it would confuse lots of users. After all you are the most familiar people with that piece of software.

jhenderson added a comment.EditedMar 1 2019, 1:35 AM

So we didn't have any tests that verified the assertion? That was an oversight.
FTR, the CHECK: {{^$}} is ensuring the first line is empty? I see that CHECK-EMPTY isn't allowed as the first directive, unfortunately.

Yes, in this particular case, that's what it is doing (although the issue also comes up for any empty character match e.g. "{{^}}"). As noted, CHECK-EMPTY can't work there, although we could possibly make a special case for CHECK-EMPTY as the first check meaning check that that first line is empty. When I implemented CHECK-EMPTY, I didn't add this special-case behaviour, as my aim was to do a CHECK-NEXT: {{^$}} that didn't accidentally match the end of the previous line.

I don't think it's wise to allow CHECK-SAME or CHECK-NEXT as the first match, due to the ambiguity that might cause as you two have been discussing. I don't have an intuitive idea of which it should be, if I'm honest.

@probinson/@jdenny, can I just confirm that you're happy with this patch going in? I just want to make sure given the chatter re. CHECK-NEXT/CHECK-SAME...

jdenny added a comment.EditedMar 1 2019, 9:56 AM

Here's how I see it, but I realize it's subjective. If we say that an initial CHECK-SAME matches the first line, then, to be consistent with the relationship CHECK-SAME and CHECK-NEXT have everywhere else, an initial CHECK-NEXT should skip a new line and thus match the second line. That means that, in order to match consecutive lines starting with the first line, we have:

CHECK-SAME: first line
CHECK-NEXT: second line
CHECK-NEXT: third line

That matches my intuition.

But the following seems more intuitive to me:

CHECK-NEXT: first line
CHECK-NEXT: second line
CHECK-NEXT: third line

To say it more abstractly:

  • CHECK-NEXT means match on the next line after the previous match.
  • CHECK-SAME means match on the same line as the previous match.

If there is no previous match because this is the first directive, then that translates to:

  • CHECK-NEXT means match on the next line after no match yet, and that intuitively sounds like the first line to me.
  • CHECK-SAME means match on the same line as no match yet, but that sounds impossible to me.

I would say it this way:

  • CHECK-NEXT means match on the next line after the starting point
  • CHECK-SAME means match on the same line as the starting point

which is more in line with the search-range model.

You feel an initial CHECK-NEXT should target the second line, right? I agree that is more in line with the formal model we have now: CHECK-NEXT requires a single newline to appear in the search range before the match.

By that reasoning, an initial CHECK-EMPTY should also target the second line: CHECK-EMPTY skips a newline in the search range before trying to match an empty line. Perhaps I misunderstood you, but I don't think that matches the intuition you expressed in your first post. Moreover, it feels unintuitive to me that you must use CHECK-SAME: {{^$}} not CHECK-EMPTY to check for an empty first line.

And what happens after CHECK-LABEL? CHECK-SAME matches on the line of the label, but CHECK-NEXT/CHECK-EMPTY requires/skips a newline before the match, so they target the first line after the label. All that's true upstream already.

It seems conceptually inconsistent to me that CHECK-NEXT/CHECK-EMPTY should target the first line after a label but the second line at the beginning of the input. I also recall your formal model says there's an implicit label at the beginning of the input, and that highlights this inconsistency more in my mind.

Again, this is a conceptual inconsistency, and I understand that it's actually consistent with the formal model. What I'm suggesting is that these directives are conceptually defined (based on their names) in terms of the previous match, so we should adjust the formal model to make sense for the case when there is no previous match (or the previous match is an implicit label before the first line): CHECK-SAME is then undefined, and CHECK-NEXT/CHECK-EMPTY don't require/skip a newline in that case because the next line after no matches is the first line.

If we are talking about matching a specific line (in this case the first line), I wonder if we could not extend the syntax of directives to accept a line number, e.g. CHECK-EMPTY-L1. Or if we think only the first line deserve to be checked explicitely simply a CHECK-FIRST. The fact that we are arguing about what would the meaning of CHECK-SAME and CHECK-NEXT on the first line be suggests that it would confuse lots of users. After all you are the most familiar people with that piece of software.

You might be right that, to avoid confusion, we need another solution altogether. I assume your proposed directives would report errors if the previous match was already past the specified line. What would be the semantics after a CHECK-LABEL or CHECK-DAG? Do we use CHECK-FIRST at the beginning of the input but CHECK-NEXT after a CHECK-LABEL?

I don't think it's wise to allow CHECK-SAME or CHECK-NEXT as the first match, due to the ambiguity that might cause as you two have been discussing. I don't have an intuitive idea of which it should be, if I'm honest.

One nice property of my interpretation is that an initial CHECK-SAME is an error. Thus, if we implement that but a user assumes Paul's interpretation of an initial CHECK-SAME (and I can definitely foresee users doing that), FileCheck must always supply a diagnostic, and that diagnostic could suggest that they actually want CHECK-NEXT. The confusion is easily resolved.

The trouble with implementing my interpretation would be if a user assumes Paul's interpretation of an initial CHECK-NEXT/CHECK-EMPTY. But how often do people actually want to ignore the first line of input and demand the initial match be specifically on line two? It seems like an unlikely use case to me, and it seems even more unlikely people would try to do this with an initial CHECK-NEXT or CHECK-EMPTY directive, so I'm not sure there's much potential for confusion. But it's hard to know for sure.

@probinson/@jdenny, can I just confirm that you're happy with this patch going in? I just want to make sure given the chatter re. CHECK-NEXT/CHECK-SAME...

I'm afraid I haven't examined the history of those asserts carefully to determine if there's a correct version to replace them. I probably need to first understand what semantics we want for these initial CHECK-{NEXT,SAME,EMPTY} directives. So, for now, I'm mostly neutral about your patch but lean toward pushing it if the asserts are causing you trouble. Of course, we should also see what Paul thinks.

I think there is value in having a way to check the very first line of input. Once we've done that, everything else ought to fall out naturally, if the model is defined well enough.

James discovered that the existing assertions interfered with ways to check that the first line is actually empty. The assertions were there mainly because we punted defining how SAME and NEXT would work at the top of the input. And, it turns out, the assertions were implemented based on an incorrect assumption anyway, triggering on a NEXT that wasn't actually the first directive.

I think the difference between Joel's and my interpretations are whether the very first line counts as the "same line" as the implicit label at the top of the input, or the "next line." I have this naive idea that a "next line" is "the one after the next newline" and there isn't an implicit newline at the top-of-input. An explicit CHECK-LABEL doesn't consume a line, which is why CHECK-SAME after a CHECK-LABEL works on the same line as the label text. So, I think the implicit label at the top-of-input works the same way, and the first line isn't a "next" line.

This all lets me keep thinking that a CHECK-SAME as the first directive would naturally look for a match on the first line of input (i.e., the "same" line as the implicit initial label), and an initial CHECK-NEXT would (by virtue of how it is defined relative to the model, not because it's inherently useful) match on the second line of input.

I can see some usefulness to saying we don't allow an initial CHECK-NEXT, because you probably meant CHECK-SAME (because your intuition matches Joel's!). This in turn means the assertion relative to CHECK-NEXT (that James wants to remove) isn't a bad idea, we just need to make it not depend on an incorrect assumption. And if for some reason you *do* want to check something on the second line, and not on the first line, you can do this:

CHECK-SAME: {{$}}
CHECK-NEXT: stuff that's on the second line

That is, I think it's correct to remove the CHECK-SAME assertion, and we need to fix the CHECK-NEXT assertion.

Sudden thought: Why are these assertions anyway? Shouldn't they be normal diagnostics? Or does FileCheck always have assertions on?

Sudden thought: Why are these assertions anyway? Shouldn't they be normal diagnostics? Or does FileCheck always have assertions on?

There are already normal diagnostics preventing the first check being a CHECK-NEXT or CHECK-SAME, so we don't need these assertions at this point, as the diagnostics have already happened. If we remove the assertions as I propose, and decide to let CHECK-SAME be usable on the first line, then those earlier diagnostics will still want to check that CHECK-NEXT is not used there (i.e. remain unchanged for CHECK-NEXT), as they do now.

thopre added a comment.Mar 7 2019, 1:19 AM

This all lets me keep thinking that a CHECK-SAME as the first directive would naturally look for a match on the first line of input (i.e., the "same" line as the implicit initial label), and an initial CHECK-NEXT would (by virtue of how it is defined relative to the model, not because it's inherently useful) match on the second line of input.

It might be consistent with the model but I think we should use a new directive altogether to clear any possible confusion. I think many are likely to only have an approximate understanding of the FileCheck model and would be surprised of using CHECK-SAME or CHECK-NEXT for the first line either way. @jdenny My suggestion of CHECK-FIRST and CHECK-EMPTY was misplaced since I am not sure what is the use case here so I'll wait clarification on that before I reply you on how such directives should behave wrt. other directives. @jhenderson Is your goal with the testcase you wrote this patch for to match a line at an absolute line number (eg line 1 in this case) or to match a line at a given line offset from the beginning of the input or region of input? My clumsily suggested CHECK-EMPTY-L1 would be for the latter case and CHECK-FIRST for the former case (and should be CHECK-EMPTY-FIRST for consistency, not sure how to generalize to any relative line number).

@jhenderson Is your goal with the testcase you wrote this patch for to match a line at an absolute line number (eg line 1 in this case) or to match a line at a given line offset from the beginning of the input or region of input? My clumsily suggested CHECK-EMPTY-L1 would be for the latter case and CHECK-FIRST for the former case (and should be CHECK-EMPTY-FIRST for consistency, not sure how to generalize to any relative line number).

In this particular case, it's the very first line, as otherwise I could use one of the other CHECK directives to get the same goal (e.g. CHECK-EMPTY).

OK sounds like we should invent a CHECK-FIRSTLINE directive. (Let's not call it CHECK-FIRST because people could think it means "do these first before other checks.") It must be the first directive, and its pattern must match on the first line of input (before the first newline).
Does that satisfy the requirements?

jhenderson added a comment.EditedMar 8 2019, 1:39 AM

OK sounds like we should invent a CHECK-FIRSTLINE directive. (Let's not call it CHECK-FIRST because people could think it means "do these first before other checks.") It must be the first directive, and its pattern must match on the first line of input (before the first newline).
Does that satisfy the requirements?

I think it would for this case, yes. I think the assertions still need removing though, since we can reach them still e.g. via empty matches, which CHECK-FIRSTLINE doesn't prevent, since people aren't forced to use it - in fact, I suspect a basic implementation of it could itself cause the same assertion, due to somebody writing something like CHECK-FIRSTLINE: {{(Possible Prefix)?}} followed by a CHECK-SAME or CHECK-NEXT.

One question about CHECK-FIRSTLINE - is it required to be the first CHECK in the file? In other words, to check two different things on the first line, would the following be allowed:

CHECK-FIRSTLINE: some stuff
CHECK-FIRSTLINE: more stuff

I don't think it needs to be, because the following would make sense to me:

CHECK-FIRSTLINE: some stuff
CHECK-SAME: more stuff

But the latter has the downside that, using multiple check prefixes, I can't say something like "check that the first line starts with some blob in one version, followed by the second blob, but in the second blob, only check for the latter", i.e. neither of the following would work:

V1ONLY-FIRSTLINE: some stuff
BOTH-SAME: more stuff
V1ONLY-FIRSTLINE: some stuff
BOTH-FIRSTLINE: more stuff

One question about CHECK-FIRSTLINE - is it required to be the first CHECK in the file? In other words, to check two different things on the first line, would the following be allowed:

CHECK-FIRSTLINE: some stuff
CHECK-FIRSTLINE: more stuff

I don't think it needs to be, because the following would make sense to me:

CHECK-FIRSTLINE: some stuff
CHECK-SAME: more stuff

The second example feels right to me. I think the reason is that my intuition still says we're doing something like CHECK-NEXT here. Thus, it's always the first match on the line, so subsequent matches on the line must be specified using CHECK-SAME.

But the latter has the downside that, using multiple check prefixes, I can't say something like "check that the first line starts with some blob in one version, followed by the second blob, but in the second blob, only check for the latter", i.e. neither of the following would work:

V1ONLY-FIRSTLINE: some stuff
BOTH-SAME: more stuff
V1ONLY-FIRSTLINE: some stuff
BOTH-FIRSTLINE: more stuff

You can handle that without new directives (after removing the broken assertions):

BOTH: {{^}}
V1ONLY-SAME: some stuff
BOTH-SAME: more stuff

Are we planning to say that an initial empty line must be matched with CHECK-FIRSTLINE: {{^$}}?

It seems we're creating yet another directive with new quirks that must be learned to use it correctly. And yet, when formally specified, it's exactly the same as CHECK-SAME except that it can be and can only be the initial directive. For those reasons, Paul's interpretation of initial CHECK-SAME (including rejection of initial CHECK-NEXT/CHECK-EMPTY with diagnostics pointing to CHECK-SAME) seems simpler and thus more user-friendly than introducing CHECK-FIRSTLINE.

Whether we choose CHECK-FIRSTLINE or Paul's interpretation of initial CHECK-SAME, I still wouldn't like the asymmetry that a {{^$}} pattern must be used to match an empty first line while CHECK-EMPTY must be used to match other consecutive lines that are empty. I'd also be disappointed that a CHECK-DAG-NEXT block, which we proposed in another review, couldn't be used to match all (unordered) lines of input.

Are we planning to say that an initial empty line must be matched with CHECK-FIRSTLINE: {{^$}}?

It seems we're creating yet another directive with new quirks that must be learned to use it correctly. And yet, when formally specified, it's exactly the same as CHECK-SAME except that it can be and can only be the initial directive. For those reasons, Paul's interpretation of initial CHECK-SAME (including rejection of initial CHECK-NEXT/CHECK-EMPTY with diagnostics pointing to CHECK-SAME) seems simpler and thus more user-friendly than introducing CHECK-FIRSTLINE.

Whether we choose CHECK-FIRSTLINE or Paul's interpretation of initial CHECK-SAME, I still wouldn't like the asymmetry that a {{^$}} pattern must be used to match an empty first line while CHECK-EMPTY must be used to match other consecutive lines that are empty. I'd also be disappointed that a CHECK-DAG-NEXT block, which we proposed in another review, couldn't be used to match all (unordered) lines of input.

I think CHECK-FIRSTLINE: {{^$}} would be a natural consequence of not wanting to change the permitted uses of other check directives, although I definitely agree it's not ideal. I'm not sure how else it would work if CHECK-SAME remained illegal as the first CHECK, and if we don't want to change CHECK-EMPTY's semantics. I suppose we could special-case CHECK-EMPTY's behaviour when it is the first check in the file to match the first line. This would certainly be intuitive, but it means that CHECK-EMPTY would no longer mean the same as CHECK-NEXT (except matching an empty line) in all cases. Thus CHECK-EMPTY becomes "Checks if the NEXT line in the input is completely empty. If it is the first specified CHECK, it checks that the first line matches is empty." I think this would be nicer than a new directive for this specific case, or to force regular expressions etc.

I think CHECK-FIRSTLINE: {{^$}} would be a natural consequence of not wanting to change the permitted uses of other check directives, although I definitely agree it's not ideal. I'm not sure how else it would work if CHECK-SAME remained illegal as the first CHECK, and if we don't want to change CHECK-EMPTY's semantics. I suppose we could special-case CHECK-EMPTY's behaviour when it is the first check in the file to match the first line. This would certainly be intuitive, but it means that CHECK-EMPTY would no longer mean the same as CHECK-NEXT (except matching an empty line) in all cases. Thus CHECK-EMPTY becomes "Checks if the NEXT line in the input is completely empty. If it is the first specified CHECK, it checks that the first line matches is empty." I think this would be nicer than a new directive for this specific case, or to force regular expressions etc.

I think I can be convinced that it is ok to make an initial CHECK-NEXT match the first line of input if an explicit mention to this case is added to the existing documentation for CHECK-NEXT and perhaps also reword the current text slightly to indicate that CHECK-NEXT checks for a match in the next line of input (so that the explicit mention of CHECK-NEXT as first directive feels natural to whoever is reading).

I think I can be convinced that it is ok to make an initial CHECK-NEXT match the first line of input if an explicit mention to this case is added to the existing documentation for CHECK-NEXT and perhaps also reword the current text slightly to indicate that CHECK-NEXT checks for a match in the next line of input (so that the explicit mention of CHECK-NEXT as first directive feels natural to whoever is reading).

I find CHECK-NEXT as the first directive to be very jarring and un-intuitive.

Has anyone raised a solid objection to this patch? If not, I see no reason to delay it further. We can always add replacement assertions later.

This patch does not enable initial CHECK-{SAME,NEXT,EMPTY}. I don't recall a pressing need for a directive that matches only the first line, and we don't seem to be converging on whether it should be one of those or some new directive. Maybe we should take a step back for a while and try this discussion again later.

Has anyone raised a solid objection to this patch? If not, I see no reason to delay it further. We can always add replacement assertions later.

This patch does not enable initial CHECK-{SAME,NEXT,EMPTY}. I don't recall a pressing need for a directive that matches only the first line, and we don't seem to be converging on whether it should be one of those or some new directive. Maybe we should take a step back for a while and try this discussion again later.

@probinson?

If we want to discuss further, I think the mailing list is the better place for it, since it will get better visibility there, and it's only tangentially related to this change.

probinson accepted this revision.Mar 12 2019, 6:50 AM

I am convinced that the asserts are wrong, and that we currently get diags for NEXT/SAME/EMPTY at the top. So LGTM to remove the asserts.

The leading semicolons in the test are noise, please remove those. Writing FileCheck tests is tricky and I'd prefer not to have anything in them that implies FileCheck itself will ignore part of the file.

This revision was automatically updated to reflect the committed changes.