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

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

jdenny added inline comments.


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


Repository:
  rG LLVM Github Monorepo

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

https://reviews.llvm.org/D93341