[PATCH] D60386: FileCheck [6/12]: Introduce numeric variable definition

[James Henderson via Phabricator via llvm-commits]

jhenderson added a comment.

I've responded to as much as I have time for. Hopefully the rest @probinson will understand what I'm talking about.



================
Comment at: llvm/include/llvm/Support/FileCheck.h:392
+  FileCheckPattern(Check::FileCheckType Ty, FileCheckPatternContext *Context,
+                   unsigned Line)
+      : Context(Context), CheckTy(Ty), LineNumber(Line) {}
----------------
size_t here?


================
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 {
----------------
thopre wrote:
> 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?
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.


================
Comment at: llvm/lib/Support/FileCheck.cpp:1713-1714
 
+  // Dummy pattern to call parseNumericVariable.
+  FileCheckPattern P(Check::CheckPlain, this, 0);
+
----------------
thopre wrote:
> 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.
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.


Repository:
  rG LLVM Github Monorepo

CHANGES SINCE LAST ACTION
  https://reviews.llvm.org/D60386/new/

https://reviews.llvm.org/D60386