[PATCH] D60386: FileCheck [6/12]: Introduce numeric variable definition
Thomas Preud'homme via Phabricator via llvm-commits
llvm-commits at lists.llvm.org
Thu May 23 09:11:24 PDT 2019
thopre added a comment.
In D60386#1513730 <https://reviews.llvm.org/D60386#1513730>, @jhenderson wrote:
> Looked at everything up to, but not including the unit tests. Thanks for all the work on this!
And many thanks again for your patience and dedicated reviews. I apologies for all the change which I fail occasionally to carry over when resolving conflict in rebases and for the english and C++ rookie mistakes.
================
Comment at: llvm/include/llvm/Support/FileCheck.h:355
public:
explicit FileCheckPattern(Check::FileCheckType Ty,
+ FileCheckPatternContext *Context, unsigned Line)
----------------
jhenderson wrote:
> 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).
I'm not sure I understand where you're getting at. Are you suggesting that explicit was only useful when the constructor only had the Ty parameter?
================
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:
> jhenderson wrote:
> > See above comments re. capture parentheses
> >
> > pointer to corresponding -> pointer to the corresponding
> I continue to not understand the "capture parenethese number"...
Oops, forgot to change that one. See above for the explanation, can't come up with a more intelligible terminology. pcresyntax(3) manual refer to them as capturing group but I don't think you'll like that any better. Would "regex parenthesized subexpression number" be clearer?
================
Comment at: llvm/include/llvm/Support/FileCheck.h:52
/// Value of numeric variable, if defined, or None otherwise.
llvm::Optional<uint64_t> Value;
----------------
jhenderson wrote:
> Not a new thing, but does this need the `llvm::`?
No, and there's also some existing function with llvm:: prefix. Is it ok if I schedule a patch to remove all of them just after this patch (i.e. between patch 6 and 7)?
================
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 {
----------------
jhenderson wrote:
> I still don't understand what a "parenthesized subexpression number is" or what a `CaptureParen` is.
I took the naming from the regex (7) manual. It refers to a parenthesis block in a regex that capture a part of the result. E.g. foo([0-9]) would capture the digit 5 if matched against foo5. I don't mind changing it to another more explicit naming (though I like this one) but would be opposed to explaining the term here because I would not know which of the occurences of this name should have the explanation.
Any suggestion?
================
Comment at: llvm/lib/Support/FileCheck.cpp:201
+ if (parseNumericVariable(Expr, Name, false, SM)) {
+ // Error reporting done in parseNumericVariable.
+ return nullptr;
----------------
jhenderson wrote:
> 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.
Mmmh, I'll try to schedule a patch to that effect once this patch is in.
================
Comment at: llvm/lib/Support/FileCheck.cpp:633
+ for (const auto &NumericVariableDef : NumericVariableDefs) {
+ assert(NumericVariableDef.getValue().CaptureParen < MatchInfo.size() &&
+ "Internal paren error");
----------------
jhenderson wrote:
> See earlier comments about CaptureParen etc. As a result, I don't really follow what this code is doing.
Each numeric expression that defines a numeric variable will have its corresponding substring in RegExStr surrounded by parenthesis which will store the text that matches it in an entry of the MatchInfo array below. I thus check that the index (capture parenthesis number) is not higher than the number of entries and then record the text that was match in the numeric variable.
================
Comment at: llvm/lib/Support/FileCheck.cpp:643
+ SM.PrintMessage(SMLoc::getFromPointer(MatchedValue.data()),
+ SourceMgr::DK_Error, "Unable to represent numeric value");
+ }
----------------
jhenderson wrote:
> 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.
Not only. It could be a too big value (e.g. a value that would require more than 64 bits to represent). That error message covers both cases. Later on it also covers cases where the format of the number does not match the format expected (e.g. the number in the input is 0xAB but the variable has unsigned decimal format).
================
Comment at: llvm/lib/Support/FileCheck.cpp:1713-1714
+ // Dummy pattern to call parseNumericVariable.
+ FileCheckPattern P(Check::CheckPlain, this, 0);
+
----------------
jhenderson wrote:
> 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.
I'm not following. The function parseNumericVariable is an instance method so needs an object to invoke it. The reason for it is that it checks for collision between numeric and string variables among other things.
================
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.
----------------
jhenderson wrote:
> "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).
That would create a function that would be called only here and contain the same code as below, wouldn't it? parseNumericVariable check whether there is a numeric variable at the start of the string it gets as parameter and verifies that this numeric variable is allowed in the context (ie. exists if it is a use, or does not conflict with a string variable if it is a definition). In this example it correctly detects FOO in FOO+2 and strip it from the original string. It is the role of the caller to decide what to do next. parseBinop for instance accepts happily to have an operator after that, but here we only want to have a variable.
This test is effectively saying: does this start with a numeric variable and is there nothing after that, i.e. is this string only a valid numeric variable.
================
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
----------------
jhenderson wrote:
> 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?
Argh indeed.
================
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:]]
----------------
jhenderson wrote:
> Doesn't adding the ':' change the semantics of this test? Why are you doing that?
This test is divided in 3 parts: (i) the first part defines some local and global variable, (ii) the second part checks that they can be matched before a LABEL barrier irregardless of whether --enable-var-scope is used and (iii) the third and last part checks that only global variable can be matched after a LABEL barrier when using --enable-var-scope.
Prior to this test the only way to define a numeric variable was via the -D command-line option hence the first part was actually doing a variable use for numeric variable. With this patch this bit can be turned into numeric variable definition to be consistent. -D command-line option is already tested in another testfile.
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