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

[James Henderson via Phabricator via llvm-commits]

jhenderson 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:
> 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?
Yes, it's fine. Not the phrasing I would use but I don't care much about it.


================
Comment at: llvm/include/llvm/Support/FileCheck.h:60
+public:
+  /// Constructs a litteral with the specified value.
+  FileCheckExpressionLiteral(uint64_t Val) : Value(Val) {}
----------------
litteral -> literal


================
Comment at: llvm/lib/Support/FileCheck.cpp:49
+
+  // Bubble up any error (eg. undefined variable) in the recursive evaluation.
+  if (!LeftOp || !RightOp) {
----------------
eg. -> e.g. 
either "an undefined variable" or "undefined variables"


================
Comment at: llvm/unittests/Support/FileCheckTest.cpp:19
+  // Eval returns the literal's value.
+  auto Ten = FileCheckExpressionLiteral(10);
+  Expected<uint64_t> Value = Ten.eval();
----------------
Why not `FileCheckExpressionLiteral Ten(10)`?


================
Comment at: llvm/unittests/Support/FileCheckTest.cpp:25
+  // Max value can be correctly represented.
+  auto Max = FileCheckExpressionLiteral(std::numeric_limits<uint64_t>::max());
+  Value = Max.eval();
----------------
Why not `FileCheckExpressionLiteral Max(...)`?


================
Comment at: llvm/unittests/Support/FileCheckTest.cpp:33
+namespace std {
+std::ostream &operator<<(std::ostream &os, const std::set<std::string> &set) {
+  bool first = true;
----------------
Other instances of overloading `operator<<` in the LLVM unit tests tend to just be in the llvm namespace, not in std. See for example ADT/StringRefTest.cpp.

The variables in this function don't use LLVM naming style.


================
Comment at: llvm/unittests/Support/FileCheckTest.cpp:35
+  bool first = true;
+  for (const std::string &s : set) {
+    os << s;
----------------
StringRef?


================
Comment at: llvm/unittests/Support/FileCheckTest.cpp:47
+
+static void expectUndefErrors(std::set<std::string> ExpectedUndefVarNames,
+                              Error Err) {
----------------
Use `std::unordered_set`?


Repository:
  rG LLVM Github Monorepo

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

https://reviews.llvm.org/D60387