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

[James Henderson via Phabricator via llvm-commits]

jhenderson added a comment.

I've run out of steam as I started looking at this, partly because I'm getting hung up on the `bool` return indicating some kind of errors. This is to me a significant design smell, and I'd prefer a different approach, although I haven't got any concrete suggestions.



================
Comment at: llvm/lib/FileCheck/FileCheck.cpp:1376
   }
   for (const auto &VariableDef : NumericVariableDefs) {
     VarCapture VC;
----------------
Aside: this is a good example of where `auto` is hurting readability of the code. I can't tell whether `VariableDef` is an `Optional`, or something else with a `getValue()` method below. If it's the former, you could replace `getValue()` with `->` for example.


================
Comment at: llvm/lib/FileCheck/FileCheck.cpp:2012
 
-static void PrintMatch(bool ExpectedMatch, const SourceMgr &SM,
+static bool printMatch(bool ExpectedMatch, const SourceMgr &SM,
                        StringRef Prefix, SMLoc Loc, const Pattern &Pat,
----------------
I'm really not a fan of `bool` return types to indicate an error. Does `true` indicate an error or `false`, for example? There's also nothing enforcing that the error is checked (see my "Error handling in libraries" lightning talk from LLVM Dev 2018 for more).


================
Comment at: llvm/lib/FileCheck/FileCheck.cpp:2141
+    SMRange NoteRange = SMRange(SearchRange.Start, SearchRange.Start);
+    for (const std::string &ErrorMsg : ErrorMsgs)
+      Diags->emplace_back(SM, Pat.getCheckTy(), Loc, MatchTy, NoteRange,
----------------
`StringRef`?


================
Comment at: llvm/lib/FileCheck/FileCheck.cpp:2240
+    if (!MatchResult.TheMatch) {
+      printNoMatch(/*ExpectedMatch=*/true, SM, *this, i, MatchBuffer,
+                   std::move(MatchResult.TheError), Req.VerboseVerbose, Diags);
----------------
A good example of my concern above: `printNoMatch` returns a `bool`, but you do nothing with it here. Should you? If not, why not?


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

https://reviews.llvm.org/D98086