fix Issue 14198 - [REG2.067a] Link failure with Variant

[WalterBright]

This was caused by the format functions recursively calling to!string, resulting in it not being inferred properly as pure. A specialization fixes it.

[WalterBright]

https://issues.dlang.org/show_bug.cgi?id=14198

[WalterBright]

This is a regression fix. Please review & approve ASAP.

[dnadlinger]

Can you please add a comment to toStr referencing the Bugzilla issue id, and maybe a minimal explanation? To me it's not at all obvious why there should be a specialization for bool. If there are two different sets of attributes inferred for the same function, it's a compiler bug, no question about that.

[MartinNowak]

fixup in #3044

[WalterBright]

toStr() actually needs a LOT of work. It needlessly allocates lots of memory, calls a complex function to do trivial formatting, etc. It makes CTFE slow. toStr() should not be recursive for non-recursive types. toStr(0) should not allocate memory. Why is there to!type, toImpl!type, and toStr? What is the point in having 3 functions?

It took me hours to reduce the bug down to that point. std.conv.to is absurdly over-complicated with layers and layers and layers.

It has poor unittest coverage at 94%. Floating point formatting is seriously undertested. Some functions are 100% untested, like strippedOctalLiteral(). std.conv is a foundational, critical infrastructure module, we can't be so sloppy with it.

[WalterBright]

Here's how to check unit test coverage:

dmd std\conv -unittest -main -cov
conv
type std-conv.lst

I.e. it's trivial to do.

[MartinNowak]

We should probably make coverage an acceptance criteria for pull requests in phobos.

[WalterBright]

In win32.mak, there's a minimum 'bar' for coverage tests.

[mihails-strasuns]

related, FYI https://issues.dlang.org/show_bug.cgi?id=14071

[andralex]

Is there work on introducing coverage requirements in the druntime and phobos makefiles?

[mihails-strasuns]

@andralex please check the bugzilla issue I have just posted

[mihails-strasuns]

(tl; dr: I hit the compiler ICE the very moment I tried to add some -cov to posix.mak)

[WalterBright]

Not that the ICE isn't a bug, it is, but you added -cov to the library generation. It has to be added to the unittest build instead.

And besides, -cov testing should be per-module, i.e. to test std.conv:

dmd std\conv -unittest -main -cov
conv

[MartinNowak]

We don't build unittests for a single module in phobos and druntime because we can't override existing modules in the shared library during linking.
So we build a shared library with all unittests and use a custom test_runner to test a single module (allows to parallelize tests).
I guess the cov switch belongs to the test obj generation and maybe to the linking cmd.

[mihails-strasuns]

And besides, -cov testing should be per-module

I find it rather impractical because it tends to create lot of test case duplication to achieve solid coverage. If module A is built on top of B, it is much more efficient to define primary test cases in terms of A and only add ones to B that address missing bits in coverage. Because of that using -cov for whole library build has been my default preference so far.

[burner]

dmd std/conv -unittest -main -cov

is not helping me. I got an additional distro dmd installed which gives me parse error. using ../dmd/src/dmd compiles but fails when linking because of the distro dmd/druntime/phobos install.
linking druntime/phobos git-head gives me multiple definitions ....

I could just compile and then link by hand, but thats no fun. so what am I doing wrong here.

[WalterBright]

what am I doing wrong here

Bug reports need to have enough detail that someone else can reproduce the problem.

If module A is built on top of B, it is much more efficient to define primary test cases in terms of A and only add ones to B that address missing bits in coverage.

It's a lot easier to debug things when a unittest immediately follows a function and tests it completely. Tangling it up with other modules makes things much harder to untangle (speaking from experience, and working on this bug is an example).

Because of that using -cov for whole library build has been my default preference so far.

You wrote above that wasn't working - is that a regression?

We don't build unittests for a single module in phobos and druntime because we can't override existing modules in the shared library during linking.

There is no need to run unittests for the shared library. Unittests are not for testing shared libraries, nor are they for testing the compiler's code generation. They are for testing the logic of a function (and certainly coverage testing falls into that category). I run unittests with -cov on a per-module basis whenever I work on a particular module.

[burner]

what am I doing wrong here
Bug reports need to have enough detail that someone else can reproduce the problem.

@WalterBright based on you reply I don't know what information you are missing. IMO it is clear that your example does not show which version of dmd to use, to compile the phobos file and how to link it.
Additionally, the existing of #3007 makes it clear that it is not as easy to compile and unittest, let alone do coverage analysis, on a single file of phobos (git HEAD).