Simplified TypeInfo, take 2: factor common code in #3206
Conversation
andralex
requested review from
Geod24,
MartinNowak,
schveiguy,
WalterBright and
wilzbach
as code owners
|
Thanks for your pull request, @andralex! Bugzilla referencesYour PR doesn't reference any Bugzilla issue. If your PR contains non-trivial changes, please reference a Bugzilla issue or create a manual changelog. 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 + druntime#3206" |
andralex
force-pushed
the
builtin_typeinfo
branch
from
772ff8f to
0aaf1ea
Compare
|
That's quite an impressive diff.
I would strongly suggest adding more tests before doing this transition- The current coverage is pretty low, see https://codecov.io/gh/dlang/druntime/tree/master/src/rt/typeinfo. |
|
@MoonlightSentinel yah, 2000 negative LOC is quite satisfying. I'm thinking of adding tests after this is pulled so as to not make it even more hairy. Is there a way to see coverage for this PR? |
Look for CodeCov: https://github.com/dlang/druntime/pull/3206/checks?check_run_id=1076583494 |
It would be better if these tests were based on the current implementation and merged in another PR. That should catch subtle changes in behaviour far more reliably.
Look for |
|
Actually I saw that coverage hints were right in the diff view - really nice! Added a few, let's see how it works. |
andralex
force-pushed
the
builtin_typeinfo
branch
2 times, most recently
from
f3d9de4 to
2421d94
Compare
andralex
force-pushed
the
builtin_typeinfo
branch
from
2421d94 to
f2f44bd
Compare
|
@MoonlightSentinel thanks for bringing coverage up... turns out that |
|
Bunch of changes in the generated docs because this eliminates all useless pages such as https://dlang.org/phobos/rt_typeinfo_ti_Along.html. |
andralex
force-pushed
the
builtin_typeinfo
branch
3 times, most recently
from
9a49c7f to
7eae5c7
Compare
andralex
force-pushed
the
builtin_typeinfo
branch
from
7eae5c7 to
f662007
Compare
andralex
force-pushed
the
builtin_typeinfo
branch
from
b6bea0e to
1aa6336
Compare
|
I have a couple more thoughts, but this step is good enough. Will set auto-merge. Thanks for reviewing! |
| // Reduces to `T` if `cond` is `true` or `U` otherwise. | ||
| private template Select(bool cond, T, U) | ||
| { | ||
| static if (cond) alias Select = T; | ||
| else alias Select = U; | ||
| } |
There was a problem hiding this comment.
Belongs into core.internal.traits
| } | ||
|
|
||
| static if (is(T == Base)) | ||
| override @property size_t tsize() nothrow pure |
There was a problem hiding this comment.
Attributes are redundant
| } | ||
|
|
||
| static if (is(T == Base) || RTInfo!T != RTInfo!Base) | ||
| override @property immutable(void)* rtInfo() nothrow pure const @safe |
|
@MoonlightSentinel rhanks for reviewing, see #3208 |
After working for a while on #3174, I figured I was straying increasingly in breaking compatibility territory. This PR starts from the other end - the currently hardcoded typeinfo implementations are factored into common templates.