[PATCH] D60389: FileCheck [9/12]: Add support for matching formats
James Henderson via Phabricator via llvm-commits
llvm-commits at lists.llvm.org
Thu Jul 25 06:34:19 PDT 2019
jhenderson added inline comments.
================
Comment at: llvm/docs/CommandGuide/FileCheck.rst:599
+would match ``mov r5, 0xF0F0`` and set ``REG`` to the value ``5`` and ``IMM``
+to the value ``61680``.
+
----------------
Perhaps worth stating "61680" in hex, rather than decimal.
================
Comment at: llvm/docs/CommandGuide/FileCheck.rst:601
+
+The syntax of a numeric substitution is ``[[#%<fmtspec>:<expr>]]`` where:
----------------
I'm getting confused by the syntax here not matching the syntax above for defining a numeric variable. Aren't the two essentially the same syntax, just with different parameters missing? If I'm not mistaken they can be unified to `[[#%<fmtspc>,<NUMVAR>:<expr>]]` where <expr> says what this matches against (if specified), <NUMVAR> says what numeric variable to store the result in (if specified) and fmtspec defines the format of the expression (if any) and that stored in the variable (if any).
Aside: does [[#%X]] match a hex number, but not store it?
================
Comment at: llvm/docs/CommandGuide/FileCheck.rst:607
+ is inferred from the matching format of the numeric variable(s) used by the
+ expression constraint if any and they are all the same, and default to ``%u``
+ if no numeric variable is used. In case of conflict between matching formats
----------------
"constraint, if any, and defaults to". The later sentence handles the "all the same" bit by talking of conflicts.
================
Comment at: llvm/include/llvm/Support/FileCheck.h:51
+ /// numbers.
+ unsigned Cap : 1;
+
----------------
MaskRay wrote:
> arichardson wrote:
> > I would call this `UpperCase` or `Capitalize`. If it is only ever used for hex values (I'm not sure if there are plans for e.g. uppercasing string values later on) storing an enumeration with `HexUpper` and `HexLower`, `Unsigned` values could also make sense.
> >
> > I you use the enum approach, it should also be possible to omit the Valid bitfield and use an enum value of `Implicit` or `NoFormat` instead.
> >
> > I haven't managed to review the full patch yet, but maybe it is enough to store just a single enumeration value in this class that also contains the conflict value since that seems mutually exclusive with all other boolean flags.
> Is there a specific reason that the 4 members must be bit fields, not regular `bool`s? I don't think that will waste space.
> I would call this UpperCase or Capitalize. If it is only ever used for hex values (I'm not sure if there are plans for e.g. uppercasing string values later on) storing an enumeration with HexUpper and HexLower, Unsigned values could also make sense.
I agree that an enum seems like the right way forward here, possibly with Conflict and Unspecified as other values in that enum.
================
Comment at: llvm/include/llvm/Support/FileCheck.h:67
+
+ /// \returns wildcard regexp StringRef to match any value in the format
+ /// represented by this instance.
----------------
"regexp"? I assume that should be "regular expression pattern"?
================
Comment at: llvm/include/llvm/Support/FileCheck.h:84
+/// Initializer for an expression without format.
+const FileCheckExpressionFormat FormatNone = {0, 0, 0, 0};
+/// Initializer for an expression matched as unsigned value.
----------------
Is there going to be an ODR violation here, due to these being instantiated in the header?
================
Comment at: llvm/include/llvm/Support/FileCheck.h:152
+ /// Pointer to AST of the expression.
+ std::shared_ptr<FileCheckExpressionAST> AST;
+
----------------
Why shared_ptr? Shouldn't an expression be the sole owner of its AST?
================
Comment at: llvm/include/llvm/Support/FileCheck.h:154
+
+ /// Matching format, i.e format (e.g. hex upper case letters) to use for the
+ /// value when matching it.
----------------
i.e -> i.e.
================
Comment at: llvm/include/llvm/Support/FileCheck.h:161
+ /// represented by \p AST and matching format is \p Format. If matching format
+ /// is unset (ie. no explicit or implicit matching format), set it to default
+ /// one (unsigned decimal integer).
----------------
ie. -> i.e.
Perhaps Format should take a default argument for an unsigned decimal?
The last clause ("set it to default one"...) then becomes unnecessary.
================
Comment at: llvm/include/llvm/Support/FileCheck.h:170
+
+ /// \returns effective format of this expression, ie (i) its explicit format
+ /// if any, (ii) its implicit format if any or (iii) the default format.
----------------
ie -> i.e.
(i) its explicit format, if any, otherwise (ii) its implicit format, if any, otherwise (iii) the default format.
================
Comment at: llvm/include/llvm/Support/FileCheck.h:186
/// variable whose value is known at parse time (e.g. @LINE pseudo variable)
- /// or cleared local variable.
- std::shared_ptr<FileCheckExpressionAST> ExpressionAST;
+ /// or cleared local variable. If expression is empty Expression points to a
+ /// FileCheckExpression with a null AST.
----------------
If the expression is empty, Expression points to a FileCheckExpression instance...
================
Comment at: llvm/include/llvm/Support/FileCheck.h:264
+
+ /// \returns implicit format of this AST, FormatConflict if implicit formats
+ /// of the AST's components conflict and Format none if the AST has no
----------------
if the implicit
================
Comment at: llvm/include/llvm/Support/FileCheck.h:265
+ /// \returns implicit format of this AST, FormatConflict if implicit formats
+ /// of the AST's components conflict and Format none if the AST has no
+ /// implicit format (e.g. AST is made of a single literal).
----------------
conflict, and FormatNone
================
Comment at: llvm/lib/Support/FileCheck.cpp:65
+ : AST(AST) {
+ this->Format = Format.Valid ? Format : FormatUnsigned;
+}
----------------
I believe you could do this in the initializer list:
```
: AST(AST)
, Format(Format.Valid ? Format : FormatUnsigned) {}
```
================
Comment at: llvm/lib/Support/FileCheck.cpp:92
return true;
- if (ExpressionAST != nullptr) {
- Expected<uint64_t> EvaluatedValue = ExpressionAST->eval();
+ if (Expression != nullptr && Expression->getAST() != nullptr) {
+ Expected<uint64_t> EvaluatedValue = Expression->getAST()->eval();
----------------
arichardson wrote:
> `if (Expression && Expression->getAST())` is a bit shorter and just as readable.
Readability is in the eye of the beholder. I personally find it more readable to compare against nullptr, because it is then clear that Expression is a pointer, not a boolean.
================
Comment at: llvm/test/FileCheck/numeric-defines.txt:4
-RUN: FileCheck -D#NUMVAL1=8 -D#NUMVAL2='NUMVAL1 + 4' -check-prefix CHECKNUM -input-file %s %s
-RUN: not FileCheck -D#NUMVAL2=8 -check-prefix CHECKNUM -input-file %s %s 2>&1 \
+RUN: FileCheck -D#%X,NUMVAL1=8 -D#NUMVAL2='NUMVAL1 + 4' -check-prefix CHECKNUM -input-file %s %s
+RUN: not FileCheck -D#%X,NUMVAL1=8 -D#NUMVAL2='NUMVAL1+6' -check-prefix CHECKNUM -input-file %s %s 2>&1 \
----------------
I feel like you're losing coverage here: where are command-line defined numeric variables tested with an implicit format specifier? I'd have a separate test for format specifiers, that explicitly focus on testing those and nothing else.
================
Comment at: llvm/test/FileCheck/numeric-expression.txt:39-43
+CHECK-NEXT: [[#%x, VAR2a:]]
+CHECK-NEXT: [[# %x, VAR2b:]]
+CHECK-NEXT: [[# %x , VAR2c:]]
+CHECK-NEXT: [[# %x , VAR2d :]]
+CHECK-NEXT: [[# %x , VAR2e : ]]
----------------
Do you really need every one of these test cases? It feels like the last one would be enough (and one with no spaces, which you have earlier on). Same goes for other instances like this elsewhere.
================
Comment at: llvm/test/FileCheck/numeric-expression.txt:47
+; variables defined on other lines.
+USE DEF FMT IMPL MATCH
11
----------------
Should this be USE EXPL FMT IMPL MATCH?
================
Comment at: llvm/test/FileCheck/numeric-expression.txt:124
+; Numeric expressions in implicit matching format and default matching rule using
+; variables defined on other lines
+USE IMPL FMT IMPL MATCH
----------------
Nit: missing trailing full stop.
================
Comment at: llvm/test/FileCheck/numeric-expression.txt:150
+
+; Explicit format override implicit format conflicts
+VAR USE IMPL OVERRIDE FMT CONFLICT
----------------
Nit: missing trailing full stop.
The grammar of this statement needs significant improvement. How about "Explicitly specified format can override conflicting implicit formats."
This test case should be after the one that shows that explicit format specifiers override non-conflicting implicit ones, and probably also after a test case showing what happens when they conflict.
================
Comment at: llvm/test/FileCheck/numeric-expression.txt:165
+; Numeric expressions with conversion matching format and implicit matching rule
+; using variables defined on other lines
+USE CONV FMT IMPL MATCH
----------------
Nit: missing trailing full stop.
What is a "conversion matching format"?
================
Comment at: llvm/test/FileCheck/numeric-expression.txt:177
+
+; Numeric variable definition with unsupported matching format
+RUN: not FileCheck -check-prefixes ERR,INVALID-FMT-SPEC1 -input-file %s %s 2>&1 \
----------------
Nit: missing trailing full stop.
================
Comment at: llvm/test/FileCheck/numeric-expression.txt:191
+INVALID-FMT-SPEC-MSG1-NEXT: {{I}}NVALID-FMT-SPEC1-NEXT: INVVAR1={{\[\[#%c,INVVAR1:\]\]}}
+INVALID-FMT-SPEC-MSG1-NEXT: {{^ \^$}}
+INVALID-FMT-SPEC-MSG2: numeric-expression.txt:[[#@LINE-4]]:37: error: invalid format specifier in expression
----------------
In one of the other changes, you went to some effort to improve the readability of these using --strict-whitespace, so that the '^' line up with the correct thing. Could you replicate these improvements in this patch too, please?
================
Comment at: llvm/test/FileCheck/numeric-expression.txt:294
+
+; Conflicting implicit format
+RUN: not FileCheck -check-prefixes CHECK,FMT-CONFLICT -input-file %s %s 2>&1 \
----------------
Missing trailing full stop.
================
Comment at: llvm/test/FileCheck/numeric-expression.txt:295-296
+; Conflicting implicit format
+RUN: not FileCheck -check-prefixes CHECK,FMT-CONFLICT -input-file %s %s 2>&1 \
+RUN: | FileCheck --strict-whitespace -check-prefix FMT-CONFLICT-MSG %s
+
----------------
You're being inconsistent here with your '-' versus '--' switch usage. Please standardise to one or the other throughout the test.
================
Comment at: llvm/test/FileCheck/numeric-expression.txt:302
+FMT-CONFLICT-NEXT: [[#VAR1 + VAR2]]
+FMT-CONFLICT-MSG: numeric-expression.txt:[[#@LINE-1]]:23: error: variables with conflicting format specifier: need an explicit one
+FMT-CONFLICT-MSG-NEXT: {{F}}MT-CONFLICT-NEXT: {{\[\[#VAR1 \+ VAR2\]\]}}
----------------
This message is incomplete - which variables? What were their format specifiers?
================
Comment at: llvm/test/FileCheck/string-defines-diagnostics.txt:28-29
; Invalid variable name: starts with a digit.
-RUN: not FileCheck -D10VALUE=10 --input-file %s %s 2>&1 \
-RUN: | FileCheck %s --strict-whitespace --check-prefix ERRCLIFMT
+RUN: not FileCheck -D10VALUE=10 -input-file %s %s 2>&1 \
+RUN: | FileCheck %s --strict-whitespace -check-prefix ERRCLINAME
----------------
Please re-add the second dash, to remain consistent with other tests in this file (why did you change it?)
================
Comment at: llvm/unittests/Support/FileCheckTest.cpp:47
// setValue works.
- auto FooVar = std::make_shared<FileCheckNumericVariable>(1, "FOO", nullptr);
+ auto NumVarExpr = FileCheckExpression(nullptr, FormatUnsigned);
+ auto FooVar =
----------------
`FileCheckExpression NumVarExpr(nullptr, FormatUnsigned);`
Same kind of comment goes throughout changes in this file.
================
Comment at: llvm/unittests/Support/FileCheckTest.cpp:48-49
+ auto NumVarExpr = FileCheckExpression(nullptr, FormatUnsigned);
+ auto FooVar =
+ std::make_shared<FileCheckNumericVariable>(1, "FOO", &NumVarExpr);
EXPECT_EQ("FOO", FooVar->getName());
----------------
Why make_shared and not make_unique? In fact, why is this a smart pointer at all? Why not just create this on the stack, i.e. `FileCheckNumericVariable FooVar (1, "FOO", &NumVarExpr);`
Same comment goes in all sorts of places elsewhere.
Repository:
rG LLVM Github Monorepo
CHANGES SINCE LAST ACTION
https://reviews.llvm.org/D60389/new/
https://reviews.llvm.org/D60389
More information about the llvm-commits
mailing list