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

[Thomas Preud'homme via Phabricator via llvm-commits]

thopre added inline comments.


================
Comment at: llvm/utils/FileCheck/FileCheck.cpp:666
         if (*InputFilePtr == '\n')
           Newline = true;
         else
----------------
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?


Repository:
  rG LLVM Github Monorepo

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

https://reviews.llvm.org/D93341