[PATCH] D154594: Add version comparison in BooleanExpression for lit testing

Tue Jul 11 11:06:31 PDT 2023

jdenny added a comment.

In D154594#4489052 <https://reviews.llvm.org/D154594#4489052>, @dstuttard wrote:

> REQUIRES etc are global - whereas substitutions are not - and can be changed as the script runs. What version of the substitution should apply, I guess it could be the context of the actual REQUIRES/UNSUPPORTED  (this would require more significant changes to the file though as these would require tracking in the same way as RUN and DEFINE/REDEFINE etc).

Is there any reason lit cannot require REQUIRES, XFAIL, etc. to appear before all RUN, DEFINE, and REDEFINE?

About the patch in general....

It seems these directives are heading in the same direction as RUN has been (see the PYTHON directive proposal <https://reviews.llvm.org/D154987> for an alternative in that case).  That is, we're incrementally developing our own full-blown expression language, and we still want more.  Could we use python's eval <https://docs.python.org/3/library/functions.html#eval> to accept python's syntax instead?

My only concern about using python in this case is that the syntax would likely become more verbose.  For example,

  XFAIL: target=powerpc{{.*}}, system-darwin

might become

  XFAIL-PY: re.match('powerpc.*', lit.target) or lit.has('system-darwin')

However, now I don't have to figure out what `,` means in the case of XFAIL as it's explicit in python, and I can immediately build arbitrarily complex expressions, including integer and string comparisons.  For substitutions, I'm proposing `lit.expand` in PYTHON directives, and we could use that here too.

As in the case of PYTHON directives, I feel using python would lower the learning curve for lit users, reduce the burden for lit developers and reviewers, and significantly increase the power of these expressions.  Again, the syntax would likely have to be more verbose, but that currently seems to me like a minor concern in comparison.

Repository:
  rG LLVM Github Monorepo

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

https://reviews.llvm.org/D154594