Fix issue 20848: format should work with immutable(void)[] #7556
Conversation
|
Thanks for your pull request, @quickfur! Bugzilla references
Testing this PR locallyIf you don't have a local development environment setup, you can use Digger to test this PR: dub run digger -- build "master + phobos#7556" |
std/format.d
Outdated
| @@ -3213,7 +3213,7 @@ if (is(StaticArrayTypeOf!T) && !is(T == enum) && !hasToString!(T, Char)) | |||
| private void formatValueImpl(Writer, T, Char)(auto ref Writer w, T obj, scope const ref FormatSpec!Char f) | |||
| if (is(DynamicArrayTypeOf!T) && !is(StringTypeOf!T) && !is(T == enum) && !hasToString!(T, Char)) | |||
| { | |||
| static if (is(const(ArrayTypeOf!T) == const(void[]))) | |||
| static if (is(const(ArrayTypeOf!T) : const(void[]))) | |||
There was a problem hiding this comment.
Isn't this true for any non-shared array T?
std/format.d
Outdated
| @@ -3213,7 +3213,7 @@ if (is(StaticArrayTypeOf!T) && !is(T == enum) && !hasToString!(T, Char)) | |||
| private void formatValueImpl(Writer, T, Char)(auto ref Writer w, T obj, scope const ref FormatSpec!Char f) | |||
| if (is(DynamicArrayTypeOf!T) && !is(StringTypeOf!T) && !is(T == enum) && !hasToString!(T, Char)) | |||
| { | |||
| static if (is(const(ArrayTypeOf!T) == const(void[]))) | |||
| static if (is(const(ArrayTypeOf!T) : const(void[]))) | |||
There was a problem hiding this comment.
| static if (is(const(ArrayTypeOf!T) : const(void[]))) | |
| static if (is(immutable(ArrayTypeOf!T) == immutable(void[]))) |
|
CI is not happy |
|
OK, this must be some kind of pathological compiler bug. The failing unittest stops failing when I comment out the newly-added unittest, but the bugfix itself appears to have no effect on the error. Swapping the bugfix for the original code appears to have no effect on this, but the act of adding this particular unittest triggers the failure. WTH is going on here?! |
|
Oh actually, come to think of it, now I remember something about the compiler caching the stringification of a delegate, but there's a bug where certain overloads share the same cache entry, so depending on which symbol was interrogated first, the Argh, yes, it's this bug (again, sigh): https://issues.dlang.org/show_bug.cgi?id=18269 |
|
And this same bug is blocking another Phobos PR: #5797 It's been 3 years, and still, no progress. :-( |
|
I am very tempted to just disable the offending unittest. Unittests shouldn't depend on the exact string format the compiler will generate for a particular type. |
The bug is that if `T == immutable(void)[]`, then `const(T)` does not collapse to `const(void[])`, but is actually `const(immutable(void)[])`. So a strict type equality check will fail. However, `const(T)` *is* implicitly convertible to `const(void[])`, which serves our purpose just as well. So just use that check instead.
I agree that brittle unit tests like that aren't useful. It'd be better to replace it though with a better test. |
|
Thanks @berni44 ! |
The bug is that if
T == immutable(void)[], thenconst(T)does not collapse toconst(void[]), but is actuallyconst(immutable(void)[]). So a strict type equality check will fail.However,
const(T)is implicitly convertible toconst(void[]), which serves our purpose just as well. So just use that check instead.