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

[James Henderson via Phabricator via llvm-commits]

jhenderson added a comment.

In D86222#2228193 <https://reviews.llvm.org/D86222#2228193>, @thopre wrote:

> There seems to be a number of failures like due to pattern like (taken from clang/test/CodeGen/debug-info-extern-call.c):
>
> // DECLS-FOR-EXTERN-NOT: !DICompileUnit({{.*}}retainedTypes: ![[RETTYPES:[0-9]+]]
> // DECLS-FOR-EXTERN-NOT: ![[RETTYPES]] = !{
>
> I think FileCheck should forbid variable definition in a CHECK-NOT pattern. Granted with this patch we'll catch all the uses of variables defined in CHECK-NOT but we won't catch if they are not used.

I have no idea what that is even trying to do. A definition within a CHECK-NOT doesn't make sense - either the pattern will match, enabling the capture of the variable, and FileCheck to fail (because of a positive CHECK-NOT match), or it will not match, meaning the variable can't be used. In other words, I think forbidding variable definitions in CHECK-NOT makes sense. It's probably worth bringing this up on llvm-dev and cfe-dev though, to give people a head's up/ask for opinions. There may be a way this actually is useful, but if so, I don't know it.

> This commit makes match error a failure for CHECK-NOT and changes PrintNoMatch to print all CHECK-NOT failure in a CHECK-NOT block rather than just the first one.

Are these two inherently linked, or could you split them into two separate changes?



================
Comment at: llvm/lib/Support/FileCheck.cpp:1965
+/// true if the failure was due to a match error.
+static bool printNoMatch(bool ExpectedMatch, const SourceMgr &SM,
                          StringRef Prefix, SMLoc Loc, const Pattern &Pat,
----------------
I'm not a fan on using booleans to indicate success or failure of a function, for two reasons:
1) It's easy to forget to check the result, meaning errors will be missed.
2) It's not obvious without reading whether a true indicates success or failure.

Can we use an `Error` instead?


================
Comment at: llvm/lib/Support/FileCheck.cpp:1972
   bool PrintDiag = true;
-  if (!ExpectedMatch) {
+  bool UndefVar = false;
+
----------------
I'm not a fan of how this variable is being used throughout this function - it feels like it would be easy to forget to check it when doing something or check it incorrectly, making this function hard to maintain.

Could we bail out early if an undefined variable is detected?


================
Comment at: llvm/test/FileCheck/numeric-expression.txt:168
 RUN: %ProtectFileCheckOutput \
-RUN: not FileCheck --check-prefix NUMEXPR-CONSTRAINT-NOMATCH --input-file %s %s 2>&1 \
+RUN: not FileCheck --check-prefixes CHECK,NUMEXPR-CONSTRAINT-NOMATCH --input-file %s %s 2>&1 \
 RUN:   | FileCheck --check-prefix NUMEXPR-CONSTRAINT-NOMATCH-MSG --strict-whitespace %s
----------------
Is this change related?


Repository:
  rG LLVM Github Monorepo

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

https://reviews.llvm.org/D86222