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

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

thopre added a comment.

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. Thanks and sorry I didn't progress faster on that one.



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


================
Comment at: llvm/lib/FileCheck/FileCheck.cpp:1195-1200
+    size_t MatchLen = FixedStr.size();
     size_t Pos =
         IgnoreCase ? Buffer.find_lower(FixedStr) : Buffer.find(FixedStr);
     if (Pos == StringRef::npos)
       return make_error<NotFoundError>();
+    return MatchResult(Pos, MatchLen, Error::success());
----------------
Nit


================
Comment at: llvm/lib/FileCheck/FileCheck.cpp:2092-2094
+                         int MatchedCount, StringRef Buffer, Error MatchError,
+                         bool VerboseVerbose,
+                         std::vector<FileCheckDiag> *Diags) {
----------------
Why the reordering of MatchError(s)?


================
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,
----------------
Shouldn't we delegate to printMatch to check for Req.VerboseVerbose?


================
Comment at: llvm/lib/FileCheck/FileCheck.cpp:2492
     }
     if (!Req.VerboseVerbose)
+      printMatch(/*ExpectedMatch=*/true, SM, Prefix, Pat.getLoc(), Pat, 1,
----------------
Likewise.


================
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 {
----------------
Move below above MatchResult.


================
Comment at: llvm/lib/FileCheck/FileCheckImpl.h:716
   ///
-  /// \returns the position that is matched or an error indicating why matching
-  /// failed. If there is a match, updates \p MatchLen with the size of the
-  /// matched string.
+  /// \returns returns either (1) an error resulting in no match or (2) a match
+  /// possibly with an error encountered while processing the match.
----------------



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

https://reviews.llvm.org/D98086