[PATCH] D133172: [lit] Extend DEFINE/REDEFINE with function substitutions

[Joel E. Denny via Phabricator via llvm-commits]

jdenny added a comment.

In D133172#3807800 <https://reviews.llvm.org/D133172#3807800>, @jhenderson wrote:

> 

Thanks for taking a look.

> I'll be honest, I don't like the syntax of this,

I get it.  It's not pretty.  However, it was the simplest syntax I came up with to address the use cases I had in mind.  I figured there would be a syntax debate, and I am open to suggestions.  (For reference, @MaskRay and I also discussed the syntax some starting at https://discourse.llvm.org/t/lit-run-a-run-line-multiple-times-with-different-replacements/64932/14.)

> so much so that I think I'd prefer people use the existing method of providing function arguments for these substitutions (i.e. DEFINE then REDEFINE them as needed) versus this.

For some uses cases, the existing DEFINE/REDEFINE functionality is sufficient and preferable.  However, as the new `TestingGuide.rst` entry in this patch discusses, that syntax is not sufficient for some other uses cases.

> The complex regex required in the lit scripts to make this format work also doesn't help this solution's case.

I'm not sure what you mean by "the lit scripts".  Are you referring to `TestRunner.py`?  Or are you thinking it will show up again somewhere else?

> Is the following format at all viable?
>
>   %{func(arg1, arg2)}

I tried something similar originally:

  %{func}(arg1, arg2)

However, I quickly ran into many cases where I needed `,` and `)` in my actual arguments.  For example, an argument might be a comma-delimited list of FileCheck/-verify prefixes.  An argument might contain expected output strings that contained parenthesized elements, such as `(null)`.  The above suggestion has the same problems.

Ultimately, I settled on `%,` and `%)` as delimiters.  That way, there’s no limit on what can go in an actual argument.  Moreover, they stick with `%` for introducing new special sequences. The `%if` syntax ultimately chose to stick with `%` as well.

> This would require lit to treat these kinds of substitutions specially,

I'm not sure I know what you mean.

Do you mean that, where we currently use python's `re.sub` and a regex to apply substitutions, we would, for the case of function substitutions, extend python with a hand-written parser for function argument lists?  Given the previous negative reaction to making lit more complex for `%if`, I figured that expressing the argument list syntax in a regex that can be used within lit's existing implementation for applying substitutions would be more acceptable.  I'm not sure that regex is actually more complicated than a hand-written parser would be.  I'm pretty sure the regex is at least more concise.  In any case, I'm open to rewriting if that's the direction the majority want.  Eventually, we'll reinvent m4.

Also, I don't think the possibility of implementing with a hand-written parser vs. a regex is specific to your proposed syntax.  Either is also possible for the syntax I proposed in this review.

But maybe I misunderstood your point.

> but the format including `(` and `)` in the name means this would at least be fairly easy to detect.

I don't know what you mean.  Can you explain more?


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

https://reviews.llvm.org/D133172