[PATCH] D49084: FileCheck: Add support for variable expressions
George Rimar via Phabricator via llvm-commits
llvm-commits at lists.llvm.org
Wed Jul 11 01:28:34 PDT 2018
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
More information about the llvm-commits
mailing list