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

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

jdenny added a comment.

Thanks for working on this.  I'm sorry to start reviewing so late.  I haven't digested the implementation fully yet, but I have a few comments to get started.



================
Comment at: llvm/include/llvm/Support/FileCheck.h:107
+    /// Indicates an error during match.
+    MatchError,
     /// Indicates a good match for an expected pattern.
----------------
The name `MatchError` and its comment seem too general.  It could easily describe many of the following enumerators.  I suggest:

```
    /// Indicates a match that can never succeed because the pattern is found to
    /// be undefined at match time (e.g., contains an undefined variable).
    MatchUndefined,
```

Undefined variables are probably the only such case we have right now, and maybe that will always be true, but I'm trying to ensure this can handle more cases if needed in the future.

Also, the middle paragraph in the preceding comments is now incorrect.  Here's a possible revision:

```
  /// A directive's supplied pattern is said to be either undefined, expected,
  /// or excluded depending on whether the pattern is well formed at match time
  /// and whether it must have or must not have a match in order for the 
  /// directive to succeed.  For example, a CHECK directive's pattern is 
  /// normally expected, a CHECK-NOT directive's pattern is normally excluded, 
  /// but either directive's pattern is undefined if it contains an undefined
  /// variable at match time.  The match result type MatchUndefined is for 
  /// undefined patterns.  All match result types whose names end with 
  /// "Excluded" are for excluded patterns.  All other match result types are 
  /// for expected patterns.
```

Here, I'm assuming you consistently use the new enumerator for undefined variables, regardless of whether a directive is positive or negative.  I haven't checked the implementation, but the test suite changes seems to confirm that.


================
Comment at: llvm/test/FileCheck/dump-input-annotations.txt:626
+; SUBST-POS-NEXT:check:2'0     X~~~~~~~~~~~~~~~~~~~~~ error: match error
+; SUBST-POS-NEXT:check:2'1                            uses undefined variable: "UNDEF"
 ; SUBST-POS-NEXT:>>>>>>
----------------
This test used to cover printing of variable definitions when a positive directive fails.  It no longer does.

A separate test could be added to cover that case.  But do we need to drop that info when a variable is undefined?  It could be helpful for debugging.


================
Comment at: llvm/utils/FileCheck/FileCheck.cpp:201
+  case FileCheckDiag::MatchError:
+    return MarkerStyle('X', raw_ostream::RED, "error: match error",
+                       /*FiltersAsError=*/true);
----------------
If you agree to changing `MatchError` to `MatchUndefined`, I suggest changing `error: match error` to `error: match undefined`.


Repository:
  rG LLVM Github Monorepo

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

https://reviews.llvm.org/D86222