[PATCH] D64923: [FileCheck]] Canonicalize caret location testing

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

thopre marked 2 inline comments as done.
thopre added a comment.

In D64923#1591865 <https://reviews.llvm.org/D64923#1591865>, @probinson wrote:

> 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 :-)



================
Comment at: llvm/test/FileCheck/check-not-diaginfo.txt:6
 
-DIAG:         error: CHECK-NOT: excluded string found in input
-DIAG-NEXT:    CHECK-NOT: test
-DIAG-NEXT: {{^           \^}}
-DIAG-NEXT:    note: found here
-DIAG-NEXT:    CHECK-NOT: test
-DIAG-NEXT: {{^           \^}}
+DIAG:           error: CHECK-NOT: excluded string found in input
+DIAG-NEXT:      CHECK-NOT: test
----------------
probinson wrote:
> 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.
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.


Repository:
  rG LLVM Github Monorepo

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

https://reviews.llvm.org/D64923