[PATCH] D60387: FileCheck [7/12]: Arbitrary long numeric expressions

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

thopre added inline comments.


================
Comment at: llvm/include/llvm/Support/FileCheck.h:553
+  /// class representing that operand in the AST of the expression or an error
+  /// holding a diagnostic against \p SM otherwise.
+  Expected<std::shared_ptr<FileCheckExpressionAST>>
----------------
thopre wrote:
> jhenderson wrote:
> > "against \p SM"? I'm not sure I understand this change in terminology.
> I've used it in other place already. I'm using "against" because the diagnostic relates to the buffer held by SM (it is created via SM.GetMessage)
Is my explanation satisfying or do you want to use another phrasing?


================
Comment at: llvm/lib/Support/FileCheck.cpp:722
                           if (!UndefSeen) {
-                            OS << "uses undefined variable ";
+                            OS << "uses undefined variable(s):";
                             UndefSeen = true;
----------------
jhenderson wrote:
> You've changed this message implying it can have more than one undefined variable, but I don't see any lit test case change that has more than one. You should add/update one.
There was a unit test already but it doesn't test that message indeed. I've added 1 lit test and updated another one, the former for correct substitution and the latter for undefined variables.


================
Comment at: llvm/unittests/Support/FileCheckTest.cpp:25
+
+static void expectUndefErrors(std::set<std::string> ExpectedUndefVarNames,
+                              Error Err) {
----------------
jhenderson wrote:
> `const std::set &`?
This cannot be const since erase is called on that set. Making it a reference means the set needs to be created in all the caller when now the caller can provide an initializer as is done in expectUndefError below.


Repository:
  rG LLVM Github Monorepo

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

https://reviews.llvm.org/D60387