[PATCH] D93341: [FileCheck] Do not skip end of line in diagnostics

[Joel E. Denny via Phabricator via llvm-commits]

jdenny added a comment.

I'm adding @mehdi_amini as a reviewer.  He's shown interest in `-dump-input` in the past, and I'd like another opinion on whether the changes made in this patch will be bearable to others as they will impact most FileCheck failures.



================
Comment at: llvm/utils/FileCheck/FileCheck.cpp:666
         if (*InputFilePtr == '\n')
           Newline = true;
         else
----------------
thopre wrote:
> jdenny wrote:
> > thopre wrote:
> > > jdenny wrote:
> > > > thopre wrote:
> > > > > jdenny wrote:
> > > > > > There should be a `COS << ' ';` here.
> > > > > I'm not sure this is a good idea because people might wonder why there's a space in the input. Right now the shown input matches the actual input in that regards.
> > > > > 
> > > > > I don't know what to think about the ~ marks though. How do annotations even work for a single CHECK directive that matches several lines (e.g. CHECK: foo{{[\n]}}bar)?
> > > > > I'm not sure this is a good idea because people might wonder why there's a space in the input. Right now the shown input matches the actual input in that regards.
> > > > 
> > > > Good point.  I was trying to ensure that unmatched text highlighting would also apply to newlines, but maybe it's not worth it.
> > > > 
> > > > > I don't know what to think about the ~ marks though. How do annotations even work for a single CHECK directive that matches several lines (e.g. CHECK: foo{{[\n]}}bar)?
> > > > 
> > > > Search ranges and match ranges are the same in that regard:
> > > > 
> > > > ```
> > > > <<<<<<
> > > >          1: FOO 
> > > > check:1     ^~~~
> > > >          2: BAR 
> > > > check:1     ~~~
> > > > >>>>>>
> > > > ```
> > > > 
> > > > I do not think there should be an inconsistency between whether newlines are annotated at the beginning of a range (`^`, `X`, etc.) and whether they're annotated somewhere within a range (`~`), especially at the end of a range.
> > > > 
> > > > For example, where `$[[:space:]]` is a little known way to exclusively match a newline (the `\n` in your patch summary doesn't work for me):
> > > > 
> > > > ```
> > > > $ cat check
> > > > CHECK: {{FOO[[:space:]]*}}
> > > > CHECK: {{$[[:space:]]BAR}}
> > > > 
> > > > $ FileCheck -vv check < input  |& tail -6
> > > > <<<<<<
> > > >          1: FOO
> > > > check:1     ^~~
> > > >          2: BAR
> > > > check:2     X~~ error: no match found
> > > > >>>>>>
> > > > ```
> > > > 
> > > > Why did the second directive skip the newline on line 1?  An extra tilde at the end of line 1 would make it clearer.
> > > Sorry for the late reply. I'm not sure how to read your answer. Do you agree about not putting a space at the end of each input line? I'm not sure what you refer by "unmatched text highlighting".
> > When input text has no matching pattern, FileCheck highlights it if color is enabled.  My concern was that, if we don't add a space for newlines, then that highlighting cannot happen for newlines, and that's inconsistent with the way annotations will mark newlines after this patch.
> > 
> > You pointed out that adding the space makes it look like the input actually had an extra space.  For that reason, I was agreeing not to add the space.
> > 
> > Really, I don't know whether the inconsistency is worse or the extra space is worse.  I see two possible solutions:
> > 
> > 1. Add the space, and add a bullet to `-dump-input=help` that states that each input line is printed with an extra trailing space to represent the newline for the sake of highlighting.
> > 2. Don't add the space but add a bullet to `-dump-input=help` that states that unmatched input highlighting is not shown for newlines.
> > 
> > At this point, I'd be happy if you'd pick either of the above.  We can always change it later.
> Although I'm sure a small portion of people using -dump-input have read the help (esp. since it's the default in case of failure to match), I really dislike inconsistencies. I would have liked to have a visual clue for the newline instead of a space but any character or sequence of character could be part of the input.
> 
> How about using a special color (e.g. lighter green/red/blue/etc) for newlines? It would not help for diagnostics without color but then space is invisible unless one look in an editor of sorts. What do you think?
Interesting idea.  Would we need two colors, one for a matched newline and one for an unmatched newline?

Another possibility is something like "•" (hopefully that renders as a bullet for you).  It's unlikely to occur in FileCheck input in LLVM's test suites, and people will quickly learn what it means because it will be on every line.  Of course, we'd want to pick something easy on the eyes and something likely to be visible in most viewers, but I'm not confident about either of those points.


Repository:
  rG LLVM Github Monorepo

CHANGES SINCE LAST ACTION
  https://reviews.llvm.org/D93341/new/

https://reviews.llvm.org/D93341