Details
- Reviewers
mclow.lists jroelofs EricWF
Diff Detail
Event Timeline
Not really a review, as I don't know how these tests are supposed to work, but one comment that you may well know and have made an active decision on already (but just in case not)...
| libcxx/test/utilities/meta/meta.trans/meta.trans.sign/make_signed.pass.cpp | ||
|---|---|---|
| 55 | I think the preprocessor rules mean that __ARM_SIZEOF_MINIMAL_ENUM gets the notional value 0 if it's not defined, so you can probably skip the first condition if you want. | |
The patch you have now looks good but I think a more universal solution might be better since these tests will fail whenever -f-short-enums is used. Something like:
test_make_signed<const Enum, std::conditional<sizeof(Enum) == sizeof(int), const int, const signed char>::type>()
Out of curiosity how are you getting the -fshort-enums flag into lit to test?
The patch you have now looks good but I think a more universal solution might be better since these tests will fail whenever -f-short-enums is used. Something like:
I'm not sure I understand... This works on ARM for both -fshort-enums and -fno-short-enums.
test_make_signed<const Enum, std::conditional<sizeof(Enum) == sizeof(int), const int, const signed char>::type>()
That seems better on the surface, but assumes that short enums are char sized, which might not be the case on all platforms. I think that the version that checks __ARM_SIZEOF_MINIMAL_ENUM does a better job of calling out the fact that it's a weird case.
Out of curiosity how are you getting the -fshort-enums flag into lit to test?
I need ABI compatibility with a bare-metal GCC toolchain and that toolchain defaults bare-metal AAPCS targets to -fshort-enums, so my driver sets it.
I'm not sure I understand... This works on ARM for both -fshort-enums and -fno-short-enums.
Sorry, I meant the original test fails whenever -fshort-enums is used. Not just on ARM. Your patch is definitely a lot better at calling out the weird behavior. Optimally I would like to make the workaround less ARM specific, like how wchar_t is handled.
I'm not convinced that we are going to run into cases where sizeof(Enum) is not the same as sizeof(int) or sizeof(char).
I apologize. I'm not trying to be difficult. My main concern is introducing a workaround that only a very small set of users can test and reason about. If '-fshort-enums' is causing this failure then I want to work around the switch and not how an architecture handles the switch.
Ah, ok. Now we're on the same page. I've changed my mind, I don't see a better platform agnostic way of looking for short enums than your suggestion... so I think that's the best option.
I'll make the change, and commit.
I think the preprocessor rules mean that __ARM_SIZEOF_MINIMAL_ENUM gets the notional value 0 if it's not defined, so you can probably skip the first condition if you want.