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

[Thomas Preud'homme via Phabricator via llvm-commits]

thopre added a comment.

In D60386#1504795 <https://reviews.llvm.org/D60386#1504795>, @probinson wrote:

> There's still some inconsistency in the terminology, as clearly evidenced by the name and description of the `IsNumExpr` member of `FileCheckPatternSubstitution`.  That class used to be where we'd find variables (now called pattern variables); but it has had numeric expressions crammed into it, except the comment wants to call it a substitution, and so we have "pattern" and "substitution" and "expression" all competing for clarity.
>
> I'm uncertain what to suggest.  It may be that "pattern variable" was not a great name, and instead it should be "string variable" to contrast more naturally with "numeric variable."  And then substitutions are just substitutions, which can be either a string substitution or a numeric substitution, with the numeric substitution implicitly using a numeric expression.  Following this idea, `FileCheckPatternSubstitution` would be renamed `FileCheckSubstitution` and the references to "numeric expression" within the class don't change.  I don't know that I'd go so far as to have String and Numeric subclasses of FileCheckSubstitution, the abstraction of a Substitution might not really support that.  But the naming can surely be improved.


That's a great idea, especially the "string variable" and "string substitution". I was not pleased with the name either but the only thing I thought of was regex variable which is plain wrong, I completely missed the obvious. I think there's still a confusion left with "substitution". The class represents the use of a numeric or string variable but I also use it in place to refer to the [[]] blocks (eg. "Numeric substitutions start with a '#' sign after the double brackets and also have the definition and substitution forms"). While even definition do lead to a substitution (since the square brackets are replaced by something else) I think substitution seems to refer more naturally to the use of a variable. So I want a name for the [[]] blocks but the only thing I can come up so far is substitution block. So you'd have string substitution blocks and numeric substitution blocks (or just substitution block when the distinction does not matter) which can be either string/numeric variable definition or string/numeric substitutions. Can you think of a better naming than substitution block? If not, I'll run with it.

I think I might have a go with a class hierarchy for FileCheckSubstitution as it might avoid the isNumExpr member variable, unless you think it's overkill.

> What do you think?  This is a discussion point, not a please-do-this comment.

I quite like the suggestion even if that means make an extra separate patch obviously. Naming clarity is very important to make the comments and thus the code understandable. It's going to be painful changing some of the error messages and thus the tests but it's worth it IMHO.


Repository:
  rG LLVM Github Monorepo

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

https://reviews.llvm.org/D60386