[PATCH] D49084: FileCheck: Add support for variable expressions

[George Rimar via Phabricator via llvm-commits]

grimar added a comment.

Few comments from me.



================
Comment at: utils/FileCheck/FileCheck.cpp:348
+          StringRef::size_type OpStart = Expr.find_first_of(validArithOps);
+          Name = Expr.substr(0, OpStart);
+        }
----------------
I would inline:

```
if (IsExpression)
  Name = Expr.substr(0, Expr.find_first_of(validArithOps));
```


================
Comment at: utils/FileCheck/FileCheck.cpp:354
+        if (VariableDefs.find(Name) != VariableDefs.end() && !IsExpression) {
+          unsigned VarParenNum = (--(VariableDefs[Name].end()))->first;
           if (VarParenNum < 1 || VarParenNum > 9) {
----------------
You do a lookup in the `VariableDefs` two times here, please avoid.
Also can you use vector's `back()` instead of constructions like `--(VariableDefs[Name].end())`?


================
Comment at: utils/FileCheck/FileCheck.cpp:373
+          std::make_pair(CurParen, RegExStr.size()));
       RegExStr += '(';
       ++CurParen;
----------------
You are doing a lookup in `VariableDefs` 3 times here. Please avoid.


================
Comment at: utils/FileCheck/FileCheck.cpp:463
+    return true;
+  else if (!Expr.empty()) {
+    // Invalid arithmetic operation
----------------
You do not need `else` after `return`:

```
  if (Expr.empty() && CheckOnly)
    return true;

   if (!Expr.empty()) {
     ...
```


================
Comment at: utils/FileCheck/FileCheck.cpp:507
+  if (isExpression) {
+    Name = Name.substr(0, Name.find_first_of(validArithOps));
+    auto VariableDefIterator = VariableDefs.find(Name);
----------------
You are doing a search of `validArithOps` in `Name` second time here. I would try to avoid.


================
Comment at: utils/FileCheck/FileCheck.cpp:667
+              MatchInfo[VariableDefIt.first], VariableDefIt.second));
+        }
+      }
----------------
In this block you have a StringMap `LocalVariableTable` and you perform 3 lookups on this map,
though can do that only once I think. For example like that:

```
  auto P = LocalVariableTable.insert({VariableDef.first, {}});
  if (P.second)
    P.first->second.push_back(
        {MatchInfo[VariableDefIt.first], VariableDefIt.second});
```


================
Comment at: utils/FileCheck/FileCheck.cpp:671
 
-  // Successful regex match.
-  assert(!MatchInfo.empty() && "Didn't get any match");
-  StringRef FullMatch = MatchInfo[0];
+    switch (State) {
+    case Unprocessed:
----------------
Since there are only 2 cases here, that might be better looking as IFs:

```
if (State == Unprocessed)
  State = DefProcessed;
else if (State == DefProcessed)
  State = ExprProcessed;
else
  llvm_unreachable("...");
```
  


================
Comment at: utils/FileCheck/FileCheck.cpp:690
+        MatchInfo[(--(VariableDef.second.end()))->first];
   }
 
----------------
I would suggest something without decrementing of the iterators:


```
  for (const auto &VariableDef : VariableDefs) {
    const std::pair<unsigned, unsigned > &P = VariableDef.second.back();
    assert(P.first < MatchInfo.size() && "Internal paren error");
    GlobalVariableTable[VariableDef.first] = MatchInfo[P.first];
  }
```

Also, btw this assert does not make sense here, because `MatchInfo` is the `SmallVector`
and it's `operator[]` already has an assert, so you can remove it I think.


Repository:
  rL LLVM

https://reviews.llvm.org/D49084