[PATCH] D98086: [FileCheck] Fix numeric error propagation

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

thopre added inline comments.


================
Comment at: llvm/include/llvm/FileCheck/FileCheck.h:137-141
+    /// Indicates an error for a match found for an expected or excluded
+    /// pattern.  The error is specified by \c Note, to which it should be
+    /// appropriate to prepend "error: " later.  The full match itself should be
+    /// recorded in a preceding diagnostic of a different \c MatchFound match
+    /// type.
----------------
jdenny wrote:
> thopre wrote:
> > May I suggest making it clear that this error is despite the match being successful? It was not obvious to me until I read the rest of the patch.
> Does the following wording help?
> 
> "Indicates an error while processing a match after it was found for an expected or excluded pattern."
> 
> I'm hesitant to use the word success because (1) there's an error and (2) matching is an error anyway if the pattern is excluded.
Sure, LGTM.


================
Comment at: llvm/lib/FileCheck/FileCheck.cpp:2092-2094
+                         int MatchedCount, StringRef Buffer, Error MatchError,
+                         bool VerboseVerbose,
+                         std::vector<FileCheckDiag> *Diags) {
----------------
jdenny wrote:
> thopre wrote:
> > Why the reordering of MatchError(s)?
> I was going for symmetry with `printMatch`, for which any error is included in the `MatchResult` parameter in that position.  To say it another way, that position holds the "result" in both cases.  If that doesn't look good to you, I'm fine to revert.
Ah no that's fine, I like symmetry. It just felt like a spurious change but I'm happy with the motivation.


================
Comment at: llvm/lib/FileCheck/FileCheck.cpp:2435
       MatchPos += MatchPosBuf;
-      if (Req.VerboseVerbose)
-        PrintMatch(true, SM, Prefix, Pat.getLoc(), Pat, 1, Buffer, MatchPos,
-                   MatchLen, Req, Diags);
+      if (MatchResult.TheError || Req.VerboseVerbose) {
+        if (printMatch(/*ExpectedMatch=*/true, SM, Prefix, Pat.getLoc(), Pat, 1,
----------------
jdenny wrote:
> thopre wrote:
> > Shouldn't we delegate to printMatch to check for Req.VerboseVerbose?
> There are two `printMatch` calls in this function.  The first triggers on (an error or) `Req.VerboseVerbose`, which reports `CHECK-DAG` discarded matches.  The second triggers on `!Req.VerboseVerbose` to be sure the final `CHECK-DAG` match is reported when discarded matches aren't shown.  We'd have to tell `printMatch` which case this is, but it seems easier just to keep that logic in the caller.
> 
> I'm open to changing it, but what I'm imagining doesn't seem as clean.
Ah sorry, I missed the negation in the second call. Nevermind then


================
Comment at: llvm/lib/FileCheck/FileCheckImpl.h:701
                     const FileCheckRequest &Req);
-  /// Matches the pattern string against the input buffer \p Buffer
+  /// A match that possibly includes an error.
+  struct Match {
----------------
jdenny wrote:
> thopre wrote:
> > Move below above MatchResult.
> Thanks for catching that.
> 
> Actually, this comment is left over from a previous attempt and is now wrong in either place.  That is, in the case of an error, `MatchResult` might not have a match.
> 
> The comment preceding `match` below says it better.  What if we just drop the above comment?
I'm fine with that. After all the structure is fairly self explanatory (Optional match, an error which we know can be success and it's named MatchResult)


================
Comment at: llvm/test/FileCheck/match-time-error-propagation/invalid-excluded-pattern.txt:13
+RUN: echo > %t.chk \
+RUN:      'CHECK-NOT: [[#0x8000000000000000+0x8000000000000000]] [[UNDEF-VAR]]'
+RUN: echo > %t.in '10000000000000000'
----------------
Did you intend to have two undefined variables (UNDEF and VAR)? Why not just UNDEFVAR? I don't think the parser looks anything after the minus sign.


================
Comment at: llvm/test/FileCheck/match-time-error-propagation/invalid-expected-pattern.txt:9
+RUN: echo > %t.chk \
+RUN:      'CHECK: [[#0x8000000000000000+0x8000000000000000]] [[UNDEF-VAR]]'
+RUN: echo > %t.in '10000000000000000'
----------------
Same as for CHECK-NOT testcase: UNDEFVAR


================
Comment at: llvm/test/FileCheck/match-time-error-propagation/matched-excluded-pattern.txt:10-18
+; TODO: Capturing from an excluded pattern probably shouldn't be permitted
+; because it seems useless: it's captured only if the pattern matches, but then
+; FileCheck fails.  The helpfulness of reporting overflow from that capture is
+; perhaps questionable then, but it doesn't seem harmful either.  Anyway, the
+; goal of this test is simply to exercise the error propagation mechanism for a
+; matched excluded pattern.  In the future, if we have a more interesting error
+; to exercise in that case, we should instead use it in this test, and then we
----------------
FYI I'd like to make a patch to use APInt in FileCheck to allow numbers bigger than 64bits. I've found the need while working on __builtin_isnan for long double. No timeline and I don't expect to have time on it soon but that would remove the only post-match error unless a new one is added before that. On the other hand I haven't even started that work so I think this testcase should be kept.


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

https://reviews.llvm.org/D98086