[PATCH] D60386: FileCheck [6/12]: Introduce numeric variable definition
James Henderson via Phabricator via llvm-commits
llvm-commits at lists.llvm.org
Thu May 23 06:30:44 PDT 2019
jhenderson added a comment.
Looked at everything up to, but not including the unit tests. Thanks for all the work on this!
================
Comment at: llvm/include/llvm/Support/FileCheck.h:355
public:
explicit FileCheckPattern(Check::FileCheckType Ty,
+ FileCheckPatternContext *Context, unsigned Line)
----------------
probinson wrote:
> Multiple parameters means you don't need 'explicit'.
There's still an explicit here (but it's worth noting that `explicit` does still have an effect on multi-parameter constructor).
================
Comment at: llvm/include/llvm/Support/FileCheck.h:332
+ /// Holds the capture parentheses number and pointer to corresponding
+ /// FileCheckNumericVariable class instance of all numeric variable
----------------
jhenderson wrote:
> See above comments re. capture parentheses
>
> pointer to corresponding -> pointer to the corresponding
I continue to not understand the "capture parenethese number"...
================
Comment at: llvm/include/llvm/Support/FileCheck.h:45
+/// expression. Each definition of a variable gets its own instance of this
+/// class. Variable uses share the same instance as the respective definition.
class FileCheckNumericVariable {
----------------
the respective definition -> their respective definitions
================
Comment at: llvm/include/llvm/Support/FileCheck.h:52
/// Value of numeric variable, if defined, or None otherwise.
llvm::Optional<uint64_t> Value;
----------------
Not a new thing, but does this need the `llvm::`?
================
Comment at: llvm/include/llvm/Support/FileCheck.h:61
+ FileCheckNumericVariable(size_t DefLineNumber, StringRef Name)
+ : Name(Name), Value(llvm::None), DefLineNumber(DefLineNumber) {}
+
----------------
I don't think you need to initialize Value here explicitly. The llvm::Optional default constructor should do that.
================
Comment at: llvm/include/llvm/Support/FileCheck.h:83
+ /// \returns the line number where this variable is defined.
+ unsigned getDefLineNumber() { return DefLineNumber; }
};
----------------
This should return size_t to match DefLineNumber's type.
================
Comment at: llvm/include/llvm/Support/FileCheck.h:245-246
+/// It holds the pointer to the class representing the numeric variable whose
+/// value is being defined and the parenthesized subexpression number in
+/// RegExStr to capture that value.
+struct FileCheckNumExprMatch {
----------------
I still don't understand what a "parenthesized subexpression number is" or what a `CaptureParen` is.
================
Comment at: llvm/include/llvm/Support/FileCheck.h:279
+ /// their own class instance not referenced by this table. When matching a
+ /// pattern all definitions for that pattern are recorded in
+ /// NumericVariableDefs table in the FileCheckPattern instance of that
----------------
in -> in the
================
Comment at: llvm/include/llvm/Support/FileCheck.h:386
+ /// definition is made on an earlier line to the one with this CHECK.
unsigned LineNumber;
----------------
This type should match that of the earlier DefLineNumber member.
================
Comment at: llvm/lib/Support/FileCheck.cpp:152
+ if (IsDefinition) {
+ // Detect collisions between string and numeric variable when the latter is
+ // created later than the former.
----------------
variable -> variables
================
Comment at: llvm/lib/Support/FileCheck.cpp:182
+ "numeric variable '" + NumericVariable->getName() +
+ "' defined on the same line");
+ return true;
----------------
same line -> same line as used (?)
================
Comment at: llvm/lib/Support/FileCheck.cpp:201
+ if (parseNumericVariable(Expr, Name, false, SM)) {
+ // Error reporting done in parseNumericVariable.
+ return nullptr;
----------------
Don't think this should change in this patch, but maybe this is a hint that parseNumericVariable and other similar functions should be returning an llvm::Error rather than a boolean, and for the errors to be handled much higher up the stack somewhere.
Returning booleans can result in confusion or people missing a check somewhere.
================
Comment at: llvm/lib/Support/FileCheck.cpp:256
+ const SourceMgr &SM) const {
+
+ // Parse numeric variable definition.
----------------
Nit: delete this blank line.
================
Comment at: llvm/lib/Support/FileCheck.cpp:290
+
+ // Parse numeric expression itself.
+ Expr = Expr.ltrim(SpaceChars);
----------------
Parse the numeric...
================
Comment at: llvm/lib/Support/FileCheck.cpp:385-387
// substitutes foo's value. Numeric substitution blocks start with a
- // '#' sign after the double brackets and only have the substitution form.
- // Both string and numeric variables must satisfy the regular expression
- // "[a-zA-Z_][0-9a-zA-Z_]*" to be valid, as this helps catch some common
- // errors.
+ // '#' sign after the double brackets and also have the definition and
+ // substitution forms. Both string and numeric variable names must satisfy
----------------
I'd change this sentence to be: "Numeric substitution blocks work the same way as string ones, but start with a '#' sign after the double brackets."
================
Comment at: llvm/lib/Support/FileCheck.cpp:409
- size_t VarEndIdx = MatchStr.find(":");
- if (IsNumBlock)
- MatchStr = MatchStr.ltrim(SpaceChars);
- else {
+ bool IsVarDef = false;
+ StringRef DefName;
----------------
I think it would be slightly clearer to just call this `IsDefinition`.
================
Comment at: llvm/lib/Support/FileCheck.cpp:509
+ if (IsNumBlock) {
+ struct FileCheckNumExprMatch NumExprDef = {DefinedNumericVariable,
+ CurParen};
----------------
No need for `struct` here.
================
Comment at: llvm/lib/Support/FileCheck.cpp:519
+ VariableDefs[DefName] = CurParen;
+ // Mark string variable as defined to detect collisions between
+ // string and numeric variable in parseNumericVariable and
----------------
Mark string -> Mark the string
================
Comment at: llvm/lib/Support/FileCheck.cpp:520
+ // Mark string variable as defined to detect collisions between
+ // string and numeric variable in parseNumericVariable and
+ // DefineCmdlineVariables when the latter is created later than the
----------------
variable -> variables
================
Comment at: llvm/lib/Support/FileCheck.cpp:522
+ // DefineCmdlineVariables when the latter is created later than the
+ // former. We cannot reuse GlobalVariableTable for that by populating
+ // it with an empty string since we would then loose the ability to
----------------
that -> this
================
Comment at: llvm/lib/Support/FileCheck.cpp:523
+ // former. We cannot reuse GlobalVariableTable for that by populating
+ // it with an empty string since we would then loose the ability to
+ // detect the use of an undefined variable in match().
----------------
loose -> lose
(loose == opposite of tight, lose == opposite of win)
================
Comment at: llvm/lib/Support/FileCheck.cpp:633
+ for (const auto &NumericVariableDef : NumericVariableDefs) {
+ assert(NumericVariableDef.getValue().CaptureParen < MatchInfo.size() &&
+ "Internal paren error");
----------------
See earlier comments about CaptureParen etc. As a result, I don't really follow what this code is doing.
================
Comment at: llvm/lib/Support/FileCheck.cpp:643
+ SM.PrintMessage(SMLoc::getFromPointer(MatchedValue.data()),
+ SourceMgr::DK_Error, "Unable to represent numeric value");
+ }
----------------
I'm not sure you've given the right error message here. Isn't the problem that MatchedValue does not represent a decimal integer? If that's the case, I'd go with an error message saying that.
================
Comment at: llvm/lib/Support/FileCheck.cpp:1713-1714
+ // Dummy pattern to call parseNumericVariable.
+ FileCheckPattern P(Check::CheckPlain, this, 0);
+
----------------
This seems like a bit of a weird thing to do. Why not just call the function directly? In other words, I suspect the comment should be updated.
================
Comment at: llvm/lib/Support/FileCheck.cpp:1738
+ P.parseNumericVariable(Expr, CmdlineName, true /*IsDefinition*/, SM);
+ // Expr test is needed to catch errors like FOO+2=10 where
+ // parseNumericVariable will successfully parse FOO.
----------------
"Expr test" needs to be explained more. I assume it's referring to the `Expr.empty()` line below, but perhaps better would be to outline what we're testing a bit more. Also, if `FOO+2=10` is invalid here, but the code passes, maybe that implies we should be calling a different function (which presumably would be a subset of the parseNumericVariable function).
================
Comment at: llvm/test/FileCheck/numeric-expression.txt:1
-RUN: FileCheck -D#VAR1=11 --input-file %s %s
+RUN: FileCheck -input-file %s %s
----------------
Use double-dash like the test was before here.
================
Comment at: llvm/test/FileCheck/numeric-expression.txt:11
+
+; Numeric variable definition in alternate spacing.
+DEF ALT SPC
----------------
"in alternate spacing" is a weird phrase. Perhaps "with different spacing"
================
Comment at: llvm/test/FileCheck/numeric-expression.txt:17-19
+CHECK-NEXT: [[# VAR1a:]]
+CHECK-NEXT: [[# VAR1b :]]
+CHECK-NEXT: [[# VAR1c : ]]
----------------
Do you ever use these definitions? I don't see them being used, but they should be, as otherwise you aren't really testing anything.
================
Comment at: llvm/test/FileCheck/numeric-expression.txt:31-32
-; Numeric expressions using variables defined on the command-line in alternate
+; Numeric expressions using variables defined on other lines in alternate.
; spacing
USE ALT SPC
----------------
See above comment about "in alternate spacing". Also, there's a rogue full-stop at the end of line 31, but none at the end of line 32.
================
Comment at: llvm/test/FileCheck/numeric-expression.txt:78
+; Numeric expression with unsupported operator.
+RUN: not FileCheck -D#NUMVAR=10 -check-prefix INVAL-OP -input-file %s %s 2>&1 \
+RUN: | FileCheck --strict-whitespace -check-prefix INVAL-OP-MSG %s
----------------
Put the double-dashes back, please, throughout the test.
================
Comment at: llvm/test/FileCheck/numeric-expression.txt:99
+RUN: | FileCheck --strict-whitespace -check-prefix INPUT-NUM-CONFLICT %s
+RUN: not FileCheck -DPATVAR=foobar -check-prefixes CONFLICT,CONFLICT4 -input-file %s %s 2>&1 \
+RUN: | FileCheck --strict-whitespace -check-prefix INPUT-NUM-CONFLICT %s
----------------
You've used "PAT" here, presumably as an abbreviation for "pattern", but I don't think that abbreviation makes sense any more given the recent naming changes?
================
Comment at: llvm/test/FileCheck/var-scope.txt:4
-RUN: FileCheck -D#LOCNUM=1 -D'#$GLOBNUM=1' --check-prefixes CHECK,LOCAL3,GLOBAL --input-file %s %s
-RUN: FileCheck -D#LOCNUM=1 -D'#$GLOBNUM=1' --check-prefixes CHECK,GLOBAL --enable-var-scope --input-file %s %s
-RUN: not FileCheck -D#LOCNUM=1 -D#'$GLOBNUM=1' --check-prefixes CHECK,LOCAL1 --enable-var-scope --input-file %s %s 2>&1 \
+; Reference run: variable are remain defined at all time when not using
+; --enable-var-scope option.
----------------
"variable are remain defined"
Not sure what this is saying exactly, but it isn't good English. Probably simply "variables remain..."
================
Comment at: llvm/test/FileCheck/var-scope.txt:18-19
global1
-CHECK: [[LOCAL:loc[^[:digit:]]*]][[#LOCNUM]]
-CHECK: [[$GLOBAL:glo[^[:digit:]]*]][[#$GLOBNUM]]
+CHECK: [[LOCAL:loc[^[:digit:]]*]][[#LOCNUM:]]
+CHECK: [[$GLOBAL:glo[^[:digit:]]*]][[#$GLOBNUM:]]
----------------
Doesn't adding the ':' change the semantics of this test? Why are you doing that?
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