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

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

jdenny added inline comments.


================
Comment at: llvm/lib/FileCheck/FileCheckImpl.h:232
   void log(raw_ostream &OS) const override {
-    OS << "\"";
-    OS.write_escaped(VarName) << "\"";
+    OS << "undefined variable: " << VarName;
   }
----------------
thopre wrote:
> jdenny wrote:
> > thopre wrote:
> > > jdenny wrote:
> > > > When is this function called now?
> > > It is a virtual function and thus needs to be defined. It's not called anywhere but I cannot remove it.
> > Makes sense.  So the point of the change here is, in case this function is one day called, to have a log message that is consistent with other `ErrorInfo` log messages now that old message here is no longer used by `printSubstitutions`.  Right?
> This change was made because I wanted to have a general error catcher in printNoMatch that would catch both UndefVarError and ErrorDiagnostic using the log() and message() methods from ErrorInfoBase. When changing the message to fit the current one I thought the "uses" is not really needed and is not consistent with other error message and the quotes and escaping are not needed since variable names are known to be well behaved.
> 
> However UndefVarError has no source manager attached to it and thus cannot print an error message with location info because there's no SourceMgr available in places where the error is created. Therefore I had to do this translation from UndefVarError to ErrorDiagnostic. But I've just found a way to reuse the log message (see last update) so it's all good.
Ah, looks good.


================
Comment at: llvm/test/FileCheck/dump-input/annotations.txt:633
+; SUBST-POS-NEXT:check:1'2                             with "DEF_MATCH2" equal to "def-match2"
+; SUBST-POS-NEXT:check:2'0     X error: match failed for invalid pattern
+; SUBST-POS-NEXT:check:2'1                             undefined variable: UNDEF
----------------
Please restore the correct alignment here and elsewhere in this file.

(I'm starting to think it might have been better to use `-strict-whitespace` in most of these tests.  I find myself turning it on temporarily anyway while adjusting tests because the squashed whitespace makes debugging confusing.  My original goal for leaving it off was to make test updates easier if we were to adjust formatting, but that rarely happens.  What do you think?)


================
Comment at: llvm/test/FileCheck/dump-input/annotations.txt:645
 ;
 ; FIXME: The first CHECK-NOT directive below uses an undefined variable.
 ; Because it therefore cannot match regardless of the input, the directive
----------------
This fixme can go now.


Repository:
  rG LLVM Github Monorepo

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

https://reviews.llvm.org/D86222