[PATCH] D81667: [RFC, FileCheck] Add precision to format specifier

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

thopre added inline comments.


================
Comment at: llvm/docs/CommandGuide/FileCheck.rst:776
   ``<constraint>`` is not provided. No matching constraint must be specified
   when the ``<expr>`` is empty.
 
----------------
jdenny wrote:
> When `<expr>` is empty (here or in the variable definition syntax), then the precision specifier specifies the //minimum// number of digits to be matched, right?
> 
> When `<expr>` is non-empty, then the precision specifier combined with the actual value of the expression specifies an //exact// number of digits to be matched, right?  I understand that the precision is a minimum here too, but I think it's a printing/substitution minimum not a matching/capturing minimum.
> 
> My point is that this case is a bit hard to follow.  It seems to me that the numeric substitution syntax with no `<expr>` is actually more like a variable definition syntax with no variable (and thus no `:`): there's no existing value to match against, so there's nothing to "substitute".  Instead you're capturing a new value and either saving it as a variable or discarding it.  Can we document it that way?
> 
> If so, instead of calling the first syntax "The syntax to define a numeric variable", you might call it "The syntax to capture a numeric value".  It can optionally define a numeric variable.
I like the idea of distinguishing between capturing a value and substituting a value. Good call.


================
Comment at: llvm/lib/Support/FileCheck.cpp:47
+Expected<std::string> ExpressionFormat::getWildcardRegex() const {
+  auto CreatePrecisionRegex = [this->Precision](StringRef S) {
+    return (S + Twine(Precision) + "}").str();
----------------
jhenderson wrote:
> This doesn't compile. I don't think you can use `->` in a capture list. You just need to specify `this` and then use appropriately below.
It's what I found out before I submit this diff, I must have forgotten to undo the change. Sorry about that.


Repository:
  rG LLVM Github Monorepo

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

https://reviews.llvm.org/D81667