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

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

thopre marked an inline comment as done.
thopre added a comment.

In D86222#2652919 <https://reviews.llvm.org/D86222#2652919>, @jdenny wrote:

> You say there are many test failures.  Should I hold off reviewing the test changes already in this patch, or are those fine?

Oh no you can review, those are errors in tests that this patch uncover from what I could see (I only looked at the first 10 or so). I'll mail llvm-def to ask people's help. The only thing missing are unit tests.



================
Comment at: llvm/lib/FileCheck/FileCheckImpl.h:232
   void log(raw_ostream &OS) const override {
-    OS << "\"";
-    OS.write_escaped(VarName) << "\"";
+    OS << "undefined variable: " << VarName;
   }
----------------
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.


Repository:
  rG LLVM Github Monorepo

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

https://reviews.llvm.org/D86222