Skip to content

Conversation

@wenhu1024
Copy link

related #2427

@CLAassistant
Copy link

CLAassistant commented Jun 27, 2025

CLA assistant check
All committers have signed the CLA.

@wenhu1024 wenhu1024 force-pushed the main branch 2 times, most recently from 719f353 to 0abd58b Compare June 27, 2025 04:53
@wenhu1024
Copy link
Author

Hi @shajder, since you’ve contributed to these files before #1719 , would you mind reviewing this change? Appreciate any comments or suggestions you may have!

@shajder
Copy link
Contributor

shajder commented Jun 27, 2025

LGTM

Copy link
Contributor

@bashbaug bashbaug left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I think this is fine, but when I run the conversions test suite I don't see any difference in the number of tests that are run.

I'm only running the "wimpy" tests, and only on scalars so the tests run faster, but I don't think this should matter? For completeness my command line is:

./test_conformance/conversions/test_conversions -w -1

If the same tests are run before and after this change then what does it do exactly? Thanks!

@wenhu1024
Copy link
Author

If run ./test_conversions fp_sat_xxx , the test result is always pass.

for example:

./test_conversions float_sat_int


conversions...
conversions passed
PASSED sub-test.
PASSED test.

Copy link
Member

@svenvh svenvh left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

If the same tests are run before and after this change then what does it do exactly?

The current code structure has the running logic sort of duplicated: one version (ConversionsTest) for running all conversions known to the CTS and another version (CustomConversionsTest) for running specific tests specified on the command line. This patch only affects the filtering for the latter. You can observe the effect of this patch by running e.g. test_conversions float_sat_char before and after the patch.

I'm not sure if silently ignoring the invalid conversion is desirable here, though. Wouldn't it be better to give an error if the user specifies an invalid conversion?

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

None yet

Projects

None yet

Development

Successfully merging this pull request may close these issues.

5 participants