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

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

jdenny added a comment.

In D98086#2611178 <https://reviews.llvm.org/D98086#2611178>, @thopre wrote:

> I like the idea behind it. I suppose that's what you had in mind when you wrote your comments in https://reviews.llvm.org/D86222. It should make that patch a lot simpler.

Wow.  That patch was clearly lurking in my subconscious, but my conscious mind had forgotten it.  I even replicated the `PrintNoMatch` -> `printNoMatch` change!  I did notice a strange feeling of deja vu while I was working on the patch.

> Thanks and sorry I didn't progress faster on that one.

No worries, and thanks for the reminder.  If there are ideas in that patch that belong here, please let me know.

I've replied to some comments, and I'll work on the others later.



================
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.
----------------
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.


================
Comment at: llvm/lib/FileCheck/FileCheck.cpp:2092-2094
+                         int MatchedCount, StringRef Buffer, Error MatchError,
+                         bool VerboseVerbose,
+                         std::vector<FileCheckDiag> *Diags) {
----------------
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.


================
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,
----------------
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.


================
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 {
----------------
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?


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

https://reviews.llvm.org/D98086