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

[George Rimar via Phabricator via llvm-commits]

grimar added inline comments.


================
Comment at: llvm/lib/Support/FileCheck.cpp:427
       bool IsDefinition = false;
+      bool Legacy = false;
       StringRef DefName;
----------------
probinson wrote:
> IsLineExpr
I'd suggest adding a comment.


================
Comment at: llvm/lib/Support/FileCheck.cpp:47
+  Expected<uint64_t> LeftOp = this->LeftOp->eval();
+  Expected<uint64_t> RightOp = this->RightOp->eval();
+
----------------
Doesn't look like you need `this->`.


================
Comment at: llvm/lib/Support/FileCheck.cpp:199
+    bool IsPseudo;
+    Expected<StringRef> ParseVarResult = parseVariable(Expr, IsPseudo, SM);
+    if (ParseVarResult) {
----------------
btw it was a bit confusing for me to find there is a method that take `bool IsPseudo`
and method that take `bool &IsPseudo`, i.e. that one of them can change this flag, but another - not.


================
Comment at: llvm/lib/Support/FileCheck.cpp:201
+    if (ParseVarResult) {
+      StringRef Name = *ParseVarResult;
+      return parseNumericVariableUse(Name, IsPseudo, SM);
----------------
You can avoid having this temorarily variable:

```
if (ParseVarResult)
  return parseNumericVariableUse(*ParseVarResult, IsPseudo, SM);
```

But actually it seems you also need to check the result of `Expected`,
not sure why it is assumed there is no error?


================
Comment at: llvm/unittests/Support/FileCheckTest.cpp:34
+std::ostream &operator<<(std::ostream &os, const std::set<std::string> &set) {
+  bool first = true;
+  for (const std::string &s : set) {
----------------
Seems a different variables naming style used here? `Ten`, `Value` vs `first`, `set`. You need stick to one I think.
(To upper case I guess? LLVM did not do a switch to a new one yet, except LLD project I believe).


Repository:
  rG LLVM Github Monorepo

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

https://reviews.llvm.org/D60387