1719959 - static_assert failure on OpenBSD/i386 On TestVariant.cpp: "Expected small index type"

Status: RESOLVED FIXED

(Core :: MFBT, defect)

Description

[Landry Breuil (:gaston)]

Probably since bug #1621865, the native build fails on 32-bits platforms, as can be seen on http://buildbot.rhaalovely.net/nine/#/builders/1/builds/1091/steps/8/logs/stdio

mfbt/tests/TestVariant.cpp:809:5: error: static_assert failed due to requirement 'sizeof (aIndex) < sizeof(unsigned long)' "Expected small index type"
162:07.17     static_assert(sizeof(aIndex) < sizeof(size_t), "Expected small index type");

Comment 1

[Gerald Squelart (he/him) (not at Mozilla since 2022-09-15)]

Thank you for the report.

I believe the type of aIndex should be uint_fast8_t (it's chosen here), but this test doesn't account for the possibility that uint_fast8_t could be as big as size_t on some systems! 😅

Landry, could you please try to change the failing static_assert in the test to:

    static_assert(is_same_v<decltype(aIndex), uint_fast8_t>);

That's a better test anyway.

If it works, I'll prepare a patch.

Comment 2

[Landry Breuil (:gaston)]

i dunno where is_same_v is supposed to come from but it seems that doesnt exist in clang 11 on OpenBSD.

171:56.32 /home/landry/src/m-c/mfbt/tests/TestVariant.cpp:809:19: error: use of undeclared identifier 'is_same_v'
171:56.32     static_assert(is_same_v<decltype(aIndex), uint_fast8_t>);

Comment 3

[Landry Breuil (:gaston)]

retrying with std::is_same_v .. doh.

Comment 4

[Landry Breuil (:gaston)]

mfbt/tests/TestVariant build succeeds on OpenBSD/i386 with

-    static_assert(sizeof(aIndex) < sizeof(size_t), "Expected small index type");
+    static_assert(std::is_same_v<decltype(aIndex), uint_fast8_t>);

Comment 5

[Gerald Squelart (he/him) (not at Mozilla since 2022-09-15)]

(In reply to Landry Breuil (:gaston) from comment #3)

retrying with std::is_same_v .. doh.

Haha, sorry about that, I did the same mistake when trying locally, but then forgot to update my comment here. 😅

(In reply to Landry Breuil (:gaston) from comment #4)

mfbt/tests/TestVariant build succeeds on OpenBSD/i386

Great, thank you for confirming. I'll prepare a proper patch soon.

Comment 6

[Gerald Squelart (he/him) (not at Mozilla since 2022-09-15)]

Attachment: Bug 1719959 - Better Tag type choice, fixed corresponding test - r?emilio

On some systems, uint_fast8_t may be as big as size_t! So the static_assert(sizeof(aIndex) < sizeof(size_t)) could fail there. The better test here is to check for the expected type (uint_fast8_t).

Now, since uint_fast8_t can be bigger than 8 bits, we may as well choose it for variant sizes greater than 255, up to UINT_FAST8_MAX.
(The added parentheses help clang-format distinguish '<' for tests vs for templates.)

Comment 7

[Pulsebot]

Pushed by [e-mail]:
https://hg.mozilla.org/integration/autoland/rev/9ca3637f9464
Better Tag type choice, fixed corresponding test - r=emilio

Comment 8

[Atila Butkovits]

https://hg.mozilla.org/mozilla-central/rev/9ca3637f9464