[RFC][libc++][test] Improves C++ Standard filtering. #89499
Conversation
This is a proof-of-concept how we could improve the C++ language filtering in lit. There will be a Discourse post for adding feedback on the approach.
This is a proof-of-concept how we can test the script. Instead of storing the data in the script it's stored in a JSON file so a different file can be used for testing. This is related to the RFC #89499
| @@ -165,11 +165,28 @@ def getSuitableClangTidy(cfg): | |||
| default=lambda cfg: next( | |||
| s for s in reversed(_allStandards) if getStdFlag(cfg, s) | |||
| ), | |||
| actions=lambda std: [ | |||
| actions=lambda std: filter(None, [ | |||
There was a problem hiding this comment.
I think we should add Lit support for arithmetic expressions instead. @jdenny-ornl do you have thoughts on this?
There was a problem hiding this comment.
I prefer this PR's approach. It keeps lit's implementation simple and defers to individual test suites to specify how they need sorting to be done.
@mordante In the initial PR comment, would you please add a link to your discourse for the sake of people arriving here?
This is a proof-of-concept how we can test the script. Instead of storing the data in the script it's stored in a JSON file so a different file can be used for testing. This is related to the RFC #89499
This is a proof-of-concept how we can test the script. Instead of storing the data in the script it's stored in a JSON file so a different file can be used for testing. This is related to the RFC #89499
| AddFeature("<=c++03") if std <= "c++03" else None, | ||
| AddFeature("<=c++11") if std <= "c++11" else None, | ||
| AddFeature("<=c++14") if std <= "c++14" else None, | ||
| AddFeature("<=c++17") if std <= "c++17" else None, | ||
| AddFeature("<=c++20") if std <= "c++20" else None, | ||
| AddFeature("<=c++23") if std <= "c++23" else None, | ||
| AddFeature("<=c++26") if std <= "c++26" else None, |
There was a problem hiding this comment.
One issue I have with this is that it's an invitation for people to write something like REQUIRES: >c++20, and that would never be a defined feature. Similarly, people would legitimately expect >= c++23 (with a space) to work, but it won't.
I wonder if that isn't creating a footgun if we don't "fully" support these relational operators at a more fundamental level.
There was a problem hiding this comment.
If we don't want to hardcode the meaning of comparing Lit features inside BooleanExpression (which I fully understand), perhaps we could simply make this feature look less like an operator (which people will then make assumptions about). Something like:
// REQUIRES: standard-at-least(c++14)
So instead of what you have in this patch, maybe something like:
Parameter(
name="std",
choices=_allStandards,
type=str,
help="The version of the standard to compile the test suite with.",
default=lambda cfg: getDefaultStdValue(cfg),
actions=lambda std: [
AddFeature(std),
AddSubstitution("%{cxx_std}", re.sub(r"\+", "x", std)),
AddCompileFlag(lambda cfg: getStdFlag(cfg, std)),
] + [AddFeature(f"standard-at-least({s})") for s in _allStandards if s < std],
),
This is basically the same thing you're doing but we use standard-at-least(...) instead of an operator.
This is a proof-of-concept how we could improve the C++ language filtering in lit. This
Discourse post discusses this approach.