This patch is part of a patch series to add support for FileCheck
numeric expressions. This specific patch introduces support for defining
numeric variable in a CHECK directive.
This commit introduces support for defining numeric variable from a
litteral value in the input text. Numeric expressions can then use the
variable provided it is on a later line.
Copyright:
| llvm/include/llvm/Support/FileCheck.h | ||
|---|---|---|
| 245–246 ↗ | (On Diff #200888) | I still don't understand what a "parenthesized subexpression number is" or what a CaptureParen is. |
| 279 ↗ | (On Diff #200888) | in -> in the |
| 386 ↗ | (On Diff #200888) | This type should match that of the earlier DefLineNumber member. |
| 355 ↗ | (On Diff #199579) | There's still an explicit here (but it's worth noting that explicit does still have an effect on multi-parameter constructor). |
| 332 ↗ | (On Diff #199479) | I continue to not understand the "capture parenethese number"... |
| llvm/lib/Support/FileCheck.cpp | ||
| 152 ↗ | (On Diff #200888) | variable -> variables |
| 182 ↗ | (On Diff #200888) | same line -> same line as used (?) |
| 201 ↗ | (On Diff #200888) | 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. |
| 256 ↗ | (On Diff #200888) | Nit: delete this blank line. |
| 290 ↗ | (On Diff #200888) | Parse the numeric... |
| 385–387 ↗ | (On Diff #200888) | 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." |
| 409 ↗ | (On Diff #200888) | I think it would be slightly clearer to just call this IsDefinition. |
| 509 ↗ | (On Diff #200888) | No need for struct here. |
| 519 ↗ | (On Diff #200888) | Mark string -> Mark the string |
| 520 ↗ | (On Diff #200888) | variable -> variables |
| 522 ↗ | (On Diff #200888) | that -> this |
| 523 ↗ | (On Diff #200888) | loose -> lose |
| 633 ↗ | (On Diff #200888) | See earlier comments about CaptureParen etc. As a result, I don't really follow what this code is doing. |
| 643 ↗ | (On Diff #200888) | 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. |
| 1713–1714 ↗ | (On Diff #200888) | 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. |
| 1738 ↗ | (On Diff #200888) | "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). |
| llvm/test/FileCheck/numeric-expression.txt | ||
| 1 ↗ | (On Diff #200888) | Use double-dash like the test was before here. |
| 11 ↗ | (On Diff #200888) | "in alternate spacing" is a weird phrase. Perhaps "with different spacing" |
| 17–19 ↗ | (On Diff #200888) | 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. |
| 31–32 ↗ | (On Diff #200888) | 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. |
| 78 ↗ | (On Diff #200888) | Put the double-dashes back, please, throughout the test. |
| 99 ↗ | (On Diff #200888) | 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? |
| llvm/test/FileCheck/var-scope.txt | ||
| 4 ↗ | (On Diff #200888) | "variable are remain defined" Not sure what this is saying exactly, but it isn't good English. Probably simply "variables remain..." |
| 18–19 ↗ | (On Diff #200888) | Doesn't adding the ':' change the semantics of this test? Why are you doing that? |
| llvm/include/llvm/Support/FileCheck.h | ||
|---|---|---|
| 355 ↗ | (On Diff #199579) | Really? What effect does explicit have on a multi-parameter constructor? My reading of the cppreference.org description is that it used to apply to T var = {args}; but that is list-initialization starting with C++11. |
| llvm/include/llvm/Support/FileCheck.h | ||
|---|---|---|
| 355 ↗ | (On Diff #199579) | The example on https://en.cppreference.com/w/cpp/language/explicit, shows that copy-list-initialization does not consider conversion constructors marked with explicit, from C++11. Take a look at the A3, A4, B3 and B4 examples. Summary version is that A var = {arg1, arg2}; will match a constructor without explicit, but not one with, whilst A var {arg1, arg2}; will work for both. Mind you, it's not likely to actually matter in most instances, and we may not actually want the behaviour of explicit. |
| llvm/include/llvm/Support/FileCheck.h | ||
|---|---|---|
| 355 ↗ | (On Diff #199579) | Okay, C++ is just too weird. |
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.
| llvm/include/llvm/Support/FileCheck.h | ||
|---|---|---|
| 52 ↗ | (On Diff #200888) | 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)? |
| 245–246 ↗ | (On Diff #200888) | 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? |
| 355 ↗ | (On Diff #199579) | 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? |
| 332 ↗ | (On Diff #199479) | 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? |
| llvm/lib/Support/FileCheck.cpp | ||
| 201 ↗ | (On Diff #200888) | Mmmh, I'll try to schedule a patch to that effect once this patch is in. |
| 633 ↗ | (On Diff #200888) | 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. |
| 643 ↗ | (On Diff #200888) | 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). |
| 1713–1714 ↗ | (On Diff #200888) | 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. |
| 1738 ↗ | (On Diff #200888) | 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. |
| llvm/test/FileCheck/numeric-expression.txt | ||
| 99 ↗ | (On Diff #200888) | Argh indeed. |
| llvm/test/FileCheck/var-scope.txt | ||
| 18–19 ↗ | (On Diff #200888) | 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. |
I'm not likely going to have any chance to look at this again by the end of the day, and I'm away all of next week. I'm okay if this gets approved by @probinson and all my outstanding comments have been addressed for this to land without me needing to look further at it.
Is there any chance you could acknowledge whether the responses I've given to some of your remarks are satisfying?
I've responded to as much as I have time for. Hopefully the rest @probinson will understand what I'm talking about.
| llvm/include/llvm/Support/FileCheck.h | ||
|---|---|---|
| 245–246 ↗ | (On Diff #200888) | I think there's a difference between capturing groups and what this code is doing, because the rest of the check isn't a regex usually. Indeed, you could end up with actual groups inside your regex pattern e.g. [[FOO:(a group)]] which could risk confusion. Also, I feel like "group" is a far more commonly understood term in regexes. I think part of the problem is that this description refers to "RegExStr" which isn't a class member of global variable, so the class is no longer standalone in its interpretation. If you really think that you should use the same terminology as regular regexes do, call them "CaptureGroup" or "GroupNumber" or something. What is the Paren actually referring to in the variable name below, for example? The name to me implies it's recording a number to do with a parenthesis, but the number's meaning is unclear. |
| 392 ↗ | (On Diff #200999) | size_t here? |
| llvm/lib/Support/FileCheck.cpp | ||
| 1713–1714 ↗ | (On Diff #200888) | Right, what I'm saying is, could the validation logic move out of parseNumericVariable into a free function, shared by parseNumericVariable and the code here. |
| llvm/test/FileCheck/numeric-expression.txt | ||
|---|---|---|
| 32–33 ↗ | (On Diff #194481) | Ping? |
Grammar nits and one longer point, which is:
I understand where the "parenthesis group" term came from, but I think it's not appropriate here. While a CHECK line is implicitly a regex, and variables are a kind of back-reference, the syntax for variable references (use or def) is not parentheses and the content is not a "group" in any real sense, and so "parenthesis group" is an un-obvious and confusing term.
Because FileCheck uses square brackets, I propose that the least disruptive change at this point would be to call them "bracket expressions." The "bracket" part is obvious, and "expression" because (a) definitions will have a regex, and (b) numeric variable references are implicitly expressions. (Okay, still a bit of a stretch, but less so that "parenthesis group" IMO.)
WDYT?
Also let me apologize in advance for the review sparsity lately, and going forward. I was at a conference all last week, and I will be on holiday with limited internet through the end of next week.
| llvm/include/llvm/Support/FileCheck.h | ||
|---|---|---|
| 411 ↗ | (On Diff #201979) | Comma after "fails" |
| 480 ↗ | (On Diff #201979) | in which case |
Actually the parenthesis group refers to the regex being generated for a given CHECK line and fed to the regex engine. Let me give an example:
CHECK: foo [[STRVAR:([a-z]+)]] #NUMVAR:
would create the regex "foo (([a-z]+)) ([0-9]+)" and record that the value for STRVAL is captured by the parenthesis group 1 (0 captures what the whole regex matches) and NUMVAR's value is captured by the parenthesis group 3. The Regex::match function takes an array where each entry takes the matched text of the corresponding parenthesis group. The FileCheckNumExprMatch structure records for numeric variable definitions what entries to look for the values in that array so parenthesis group is perfectly adequate (apart from perhaps better naming) IMHO.
Also let me apologize in advance for the review sparsity lately, and going forward. I was at a conference all last week, and I will be on holiday with limited internet through the end of next week.
Sparsity of review is a given when reviewer is from a different company, I adapt my workflow around it. Likewise I try to respond to comments as fast as possible but might not always be able to.
Ah ha. The ParenGroup referring to the CHECK line as it has been translated for consumption by the underlying regex package... that was not clear.
In which case the terminology is okay but the commentary could be better, see inline comment.
| llvm/include/llvm/Support/FileCheck.h | ||
|---|---|---|
| 350 ↗ | (On Diff #201979) | I think what this is trying to say is: Is that right? The translation of check-line-with-square-brackets-with-regex into parenthesized-regex-passed-to-regex-library is not obvious in this comment as written. |
| llvm/include/llvm/Support/FileCheck.h | ||
|---|---|---|
| 350 ↗ | (On Diff #201979) | Indeed. The comment was already unclear before this patch series, I think the reason is in most cases the CHECK directive does not contain parenthesis so the square bracket definition maps directly to the parenthesis group number. Hopefully the comment is now clearer. |
I am pretty sure all @jhenderson comments have been addressed, and I'm happy, so LGTM.
As I said previously, it may be a while before I get to your next patch.
| llvm/include/llvm/Support/FileCheck.h | ||
|---|---|---|
| 350 ↗ | (On Diff #201979) | It looks odd that the translated expression has nothing for the use of [[bar]] in the original, but the comment is much better now, thanks! |
Fix value of RegExStr in comment about VariableDefs and show case of use of variable defined on earlier line.
| llvm/include/llvm/Support/FileCheck.h | ||
|---|---|---|
| 350 ↗ | (On Diff #201979) | Indeed, it should contain a backreference to the first definition. When it refers to a definition on a different line the value gets inserted in the regex just before match. I've updated the pattern to show both cases, using <quux value> to represent the value captured on a previous line of QUUX. |
This is a private method so I've moved the tests to ParseExpr which tests parseNumericSubstitutionBlock which calls into parseBinop (also private) which calls into parseNumericVariableUse.
Oh, my bad, I checked before making that comment, and thought it was public. Must have misread the code. LGTM.