[PATCH] D60387: FileCheck [7/12]: Arbitrary long numeric expressions
James Henderson via Phabricator via llvm-commits
llvm-commits at lists.llvm.org
Wed Jun 19 07:46:55 PDT 2019
jhenderson added a comment.
Apologies for the delay in coming back to this. It's been a busy few days.
================
Comment at: llvm/include/llvm/Support/FileCheck.h:130
/// Left operand.
- FileCheckNumericVariable *LeftOp;
+ std::shared_ptr<FileCheckNumExprAST> LeftOp;
----------------
thopre wrote:
> jhenderson wrote:
> > Do these need to be shared_ptrs? Do multiple places need ownership, or is it sufficient that only place has it? Same question applies for all uses of shared_ptr further down.
> Some AST nodes are referenced from a single place only (eg. literals and binary operations) while numeric variables can be shared among several numeric substitutions, eg. "add r[[#REG]], r[[#REG+1]], 42" would have 2 pointers to the class instance for REG. In the first case std::unique_ptr could be used, while in the latter case std::shared_ptr would make sense.
>
> The problem comes from AST nodes being referred via FileCheckAST * pointers (since each operand can be either of the possible nodes) and thus I need to decide on a given pointer type for all nodes. I could go with regular pointers and keep an array of pointer in FileCheckPatternContext to all nodes created for them to be freed all at once when matching is done. Wouldn't change much over what is happening now since substitutions are only deleted once all pattern have been matched and substitutions points to the AST indirectly. Does that sound good?
I wonder whether it might make more sense to separate the concept of variables from the AST. The AST could have a variable reference class in it, which refers by raw pointer to a variable definition. An AST expression then solely "owns" each subexpression, whilst something higher up (e.g. the Context - not sure about this), owns the variables and contains the definitions. That way each item in the tree is only owned by its parents, which feels more logical.
Does that make sense?
As things stand the fact that a single FileCheckNumericVariable can be owned by multiple places implies it's not really a tree at all. (In graph theory a tree has no cross-references - everything is owned by a single parent only).
================
Comment at: llvm/include/llvm/Support/FileCheck.h:441
+ /// Parses \p Expr for a numeric substitution block. Parameter \p Legacy
+ /// indicates whether Expr should be a legacy numeric substitution block.
+ /// Sets \p NumExprAST to point to the class instance representing the
----------------
thopre wrote:
> probinson wrote:
> > thopre wrote:
> > > jhenderson wrote:
> > > > What's a "legacy numeric substitution block"?
> > > It refers to the "legacy use of @LINE" mentioned in the section "FileCheck Pseudo Numeric Variables" of the documentation, namely [[@LINE]] and [[@LINE+offset]] instead of the same with a '#' sign. Would "indicates whether Expr is a legacy use of @LINE" be clearer? Or perhaps "legacy @LINE expression"? If not, any other suggestion?
> > I don't know about James but I would prefer that the commentary say "legacy @LINE expression" and the parameter could be named LineExpr.
> I went for LegacyLineExpr then since when false @LINE is also allowed and when true it limits what can be done with @LINE.
How about `IsLegacyLineExpr`? It's a bit more verbose but maybe a little clearer too. Otherwise `LegacyLineExpr` is okay. It's just that the variable isn't actually an expression itself.
================
Comment at: llvm/include/llvm/Support/FileCheck.h:67-68
+
+/// Class to represent an undefined variable error which prints that variable's
+/// name between quotes when printed.
+class FileCheckUndefVarError : public ErrorInfo<FileCheckUndefVarError> {
----------------
I think "Class to represent an undefined variable error, which quotes that variable's name when printed." would be a bit more concise.
================
Comment at: llvm/include/llvm/Support/FileCheck.h:94
/// class. Variable uses share the same instance as their respective
/// definition.
+class FileCheckNumericVariable : public FileCheckExpressionAST {
----------------
definition -> definitions
================
Comment at: llvm/include/llvm/Support/FileCheck.h:121
/// \returns this variable's value.
- Optional<uint64_t> getValue() const { return Value; }
+ Expected<uint64_t> eval() const;
----------------
If you made this eval() return an error if the variable is undefined, it would nicely separate concerns of use versus definition (see my comment a bit further down about shared_ptrs).
================
Comment at: llvm/include/llvm/Support/FileCheck.h:158
+ /// using EvalBinop on the result of recursively evaluating the operands.
+ /// \returns an error if a numeric variable used is undefined, or the
+ /// expression value otherwise.
----------------
This should say "returns an error if a subexpression returns an error, or ..."
================
Comment at: llvm/include/llvm/Support/FileCheck.h:480
+ /// expression. \returns a pointer to the class instance representing the AST
+ /// of the expression whose value must be substitued, or an error holding a
+ /// diagnostic against \p SM if parsing fails. If substitution was
----------------
substitued -> substituted
================
Comment at: llvm/include/llvm/Support/FileCheck.h:549
+ const SourceMgr &SM) const;
+ enum AllowedOperand { LineVar, Literal, Any };
+ /// Parses \p Expr for use of a numeric operand. Accepts both literal values
----------------
Perhaps worth making this an `enum class`.
================
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>>
----------------
"against \p SM"? I'm not sure I understand this change in terminology.
================
Comment at: llvm/lib/Support/FileCheck.cpp:198
+ uint64_t LiteralValue;
+ if (!Expr.consumeInteger(10 /*Radix*/, LiteralValue))
+ return std::make_shared<FileCheckExpressionLiteral>(LiteralValue);
----------------
I'm guessing there is a plan to allow other Radixes too? It seems like it would be normal to want to use hex, for example.
================
Comment at: llvm/lib/Support/FileCheck.cpp:243
"missing operand in expression");
- uint64_t RightOp;
- if (Expr.consumeInteger(10, RightOp))
- return FileCheckErrorDiagnostic::get(
- SM, Expr, "invalid offset in expression '" + Expr + "'");
- Expr = Expr.ltrim(SpaceChars);
- if (!Expr.empty())
- return FileCheckErrorDiagnostic::get(
- SM, Expr, "unexpected characters at end of expression '" + Expr + "'");
+ // Second operand in legacy @LINE expression is a literal.
+ AllowedOperand AO = LegacyLineExpr ? Literal : Any;
----------------
in a legacy
================
Comment at: llvm/lib/Support/FileCheck.cpp:270
StringRef Name;
Error ErrorDiagnostic =
parseNumericVariableDefinition(DefExpr, Name, Context, SM);
----------------
Err would be a more common variable name.
================
Comment at: llvm/lib/Support/FileCheck.cpp:290
+ Expr = Expr.ltrim(SpaceChars);
+ // First operand in legacy @LINE expression is the @LINE pseudo variable.
+ AllowedOperand AO = LegacyLineExpr ? LineVar : Any;
----------------
in a legacy
================
Comment at: llvm/lib/Support/FileCheck.cpp:715
[](const FileCheckNotFoundError &E) {},
// Handled in PrintNoMatch()
[](const FileCheckErrorDiagnostic &E) {},
----------------
Trailing full stop required.
================
Comment at: llvm/lib/Support/FileCheck.cpp:725-727
[](const ErrorInfoBase &E) {
llvm_unreachable("Unexpected error");
});
----------------
I think this is actually unnecessary. `handleAllErrors` terminates the program with an `llvm_unreachable` anyway if there are any unhandled errors.
Repository:
rG LLVM Github Monorepo
CHANGES SINCE LAST ACTION
https://reviews.llvm.org/D60387/new/
https://reviews.llvm.org/D60387
More information about the llvm-commits
mailing list