[PATCH] D60386: FileCheck [6/12]: Introduce numeric variable definition
Paul Robinson via Phabricator via llvm-commits
llvm-commits at lists.llvm.org
Mon May 13 10:23:02 PDT 2019
probinson added a comment.
Starting set of comments on this one. Haven't looked at everything yet.
================
Comment at: llvm/docs/CommandGuide/FileCheck.rst:623
+Important note: In its current implementation, a numeric expression cannot use
+a numeric variable defined on the same line.
+
----------------
Which makes the above example illegal, so you should rewrite the example.
================
Comment at: llvm/include/llvm/Support/FileCheck.h:379
+ /// Parses \p Expr for a numeric expression. \returns the class representing
+ /// the AST of numeric expression or nullptr if parsing fails in which case
+ /// errors are reported on \p SM. Sets \p DefinedNumericVariable to the
----------------
"of the numeric"
comma after "fails"
================
Comment at: llvm/include/llvm/Support/FileCheck.h:382
+ /// pointer to the class representing the variable defined to this numeric
+ /// expression if any.
+ FileCheckNumExpr *
----------------
That last sentence feels awkward... maybe:
"If the expression defines a numeric variable, sets \p DefinedNumericVariable to point to the class representing the variable."
(with maybe "or nullptr if it does not define a variable.")
================
Comment at: llvm/include/llvm/Support/FileCheck.h:397
bool ParsePattern(StringRef PatternStr, StringRef Prefix, SourceMgr &SM,
- unsigned LineNumber, const FileCheckRequest &Req);
+ const FileCheckRequest &Req);
/// Matches the pattern string against the input buffer \p Buffer
----------------
Need to remove LineNumber from the documentation, if it's no longer a parameter.
================
Comment at: llvm/lib/Support/FileCheck.cpp:42
llvm::Optional<uint64_t> FileCheckNumExpr::eval() const {
- llvm::Optional<uint64_t> LeftOp = this->LeftOp->getValue();
+ assert(LeftOp != nullptr && "Evaluating an empty numeric expression");
+ llvm::Optional<uint64_t> LeftOpValue = LeftOp->getValue();
----------------
Omit the `!= nullptr` on pointer assertions.
================
Comment at: llvm/lib/Support/FileCheck.cpp:170
+ // below is for the class instance corresponding to the last definition of
+ // this variable use.
auto VarTableIter = Context->GlobalNumericVariableTable.find(Name);
----------------
I think this comment should not have changed.
================
Comment at: llvm/lib/Support/FileCheck.cpp:289
+ return Context->makeNumExpr(add, nullptr, 0);
+ } else
+
----------------
no else after return
================
Comment at: llvm/lib/Support/FileCheck.cpp:388
+ // the double brackets and also have the definition and substitution forms.
+ // Both pattern and numeric variables must satisfy the regular expression
// "[a-zA-Z_][0-9a-zA-Z_]*" to be valid, as this helps catch some common
----------------
variables -> variable names
================
Comment at: llvm/lib/Support/FileCheck.cpp:412
- size_t VarEndIdx = MatchStr.find(":");
- if (IsNumExpr)
- MatchStr = MatchStr.ltrim(SpaceChars);
- else {
+ bool IsVarDef;
+ StringRef DefName;
----------------
`= false`
================
Comment at: llvm/lib/Support/FileCheck.cpp:467
- if (IsNumExpr || (!IsVarDef && IsPseudo)) {
- NumExpr = parseNumericExpression(Name, IsPseudo, Trailer, SM);
+ // Parse numeric expression.
+ if (IsNumExpr) {
----------------
I think `NumExpr` and `DefinedNumericVariable` could be declared here, much closer to where they are used.
================
Comment at: llvm/lib/Support/FileCheck.cpp:473
IsNumExpr = true;
+ if (DefinedNumericVariable != nullptr) {
+ IsVarDef = true;
----------------
Omit `!= nullptr`
================
Comment at: llvm/lib/Support/FileCheck.cpp:1794
// GlobalVariableTable for that by populating it with an empty string
- // since we would then lose the ability to detect the use of an undefined
- // variable in Match().
+ // since we would then lose the ability to detect use of undefined
+ // variable in match().
----------------
"detect the use of an undefined"
(missing rebase maybe?)
Repository:
rG LLVM Github Monorepo
CHANGES SINCE LAST ACTION
https://reviews.llvm.org/D60386/new/
https://reviews.llvm.org/D60386
More information about the llvm-commits
mailing list