[PATCH] D86222: Fix PR46880: Fail CHECK-NOT with undefined variable

Wed Dec 16 02:03:00 PST 2020

thopre added inline comments.

================
Comment at: llvm/lib/Support/FileCheck.cpp:2014
+  // the next line.
+  Buffer = Buffer.substr(Buffer.find_first_not_of(" \t\n\r"));
+  FileCheckDiag::MatchType MatchTy = FileCheckDiag::MatchError;
----------------
jhenderson wrote:
> thopre wrote:
> > jhenderson wrote:
> > > thopre wrote:
> > > > jhenderson wrote:
> > > > > This might be a dumb question, but I'm not following why we're skipping '\t' and ' ' here. It certainly doesn't line up with the comment above.
> > > > I'm not sure, this is copied over from PrintNoMatch. I thought it is used so that the "scanning from here" shows the first non blank line after last match but I haven't looked in details.
> > > Could you investigate a bit more and then update the comment with your findings? This code at the moment also skips some whitespace, which isn't always desirable (imagine if you had a regex match looking for some explicit whitespace before some string).
> > The code was added in da108b4ed4e6e7267701e76d5fd3b87609c9ab77 when CHECK-NEXT was added and at the same time as the "scanning from here" diagnostic was added. There was no CHECK-SAME at the time and I guess the goal was to avoid confusion by showing the last matching line (even though the column info would show it's the end of the line). There was no regular expression match either, so no way to match a newline.
> > 
> > I think this should simply be removed, in a separate patch. What is your opinion?
> I'm inclined to agree with you. Perhaps worth adding a TODO or FIXME note here and at the other location, referencing each other, saying that this should probably be removed?
> 
> @probinson, (or anybody else) do you have any thoughts on this?
I've created https://reviews.llvm.org/D93341. I'll rebase this patch on top of it once the approach is agreed upon.

Repository:
  rG LLVM Github Monorepo

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

https://reviews.llvm.org/D86222