Respect endianness correctly in CheckEnums test suite #143339
Conversation
rustbot
added
A-compiletest
|
Some changes occurred in src/tools/compiletest cc @jieyouxu |
|
☔ The latest upstream changes (presumably #143337) made this pull request unmergeable. Please resolve the merge conflicts. |
|
@rustbot author |
rustbot
added
S-waiting-on-author
There was a problem hiding this comment.
Why does this test use a struct with two u16 fields to begin with? if you transmuted a u32 directly to Foo, things would be endian-independent. (same for convert_non_enum_break)
There was a problem hiding this comment.
The idea of this test was to transmute a non-integer value to an enum. I am happy to change it to a u32 here, but in that case I might as well delete it because that behavior is covered by other tests. My goal here was to check that I can safely extract the discriminant from a transmute of non-int value.
There was a problem hiding this comment.
I have no idea what you think that's an interesting case to test, and there is no comment in the test explaining that this is what the test is about. Please always add such comments.
But anyway, whatever the point of the test is, surely it can be done on both little-endian and big-endian systems. So find a form of the test that does that, or add #[cfg(target_endian = ...)] in the test if you need slightly different code to make it work in both ways (and then ensure you actually test both codepaths, e.g. with Miri).
The goal should be to have the exact same test coverage on little-endian and big-endian. It makes no sense at all to take the existing little-endian tests and just update the expected output for big-endian, as they obviously don't test the right thing on big-endian if they produce such different results there!
There was a problem hiding this comment.
I have no idea what you think that's an interesting case to test
So you think it doesn't make sense to test that? My point was that I am transmuting internally between either an Int-To-Int or a direct transmute. I want to make sure that the direct transmute is working correctly and that my codepath that inserts it behaves correctly. If you disagree with that, it's better to drop the test in favor of a slimmer testsuite.
The goal should be to have the exact same test coverage on little-endian and big-endian.
Yes I understand that point, thanks for the feedback. For that reason I went ahead and changed this struct to one with just a single member that is a u32. The test is the same for big and little endian.
There was a problem hiding this comment.
I don't know your code (and unfortunately don't have the time to dig into it right now), maybe the test makes sense, but it's very unclear from the outside.^^
I went ahead and changed this struct to one with just a single member that is a u32. The test is the same for big and little endian.
Such a struct is identical to an int for most parts of the compiler. I am surprised that it would make any difference for your code. But sure, that sounds good. :)
|
I updated the test suite to always work for both cases and stopped enabling or disabling tests as per Ralf's suggestion. |
The endianness can change the test expectation for the enum check. This change is fixing the failing tests on big endian by changing the tests so that they behave the same as on little endian.
|
This looks good... @fneddy any chance you could test this, or should we just land it? |
The endianness can change the test expectation for the enum check. This change is fixing the failing tests on big endian by changing the tests so that they behave the same as on little endian.
Fixes #143332.