Jump to page: 1 2 3
Thread overview
What's the deal with the massive duplication in std.meta?
May 01, 2021
Imperatorn
May 01, 2021
Berni44
May 01, 2021
Walter Bright
May 02, 2021
Herringway
May 02, 2021
Walter Bright
May 04, 2021
Q. Schroll
May 04, 2021
Paul Backus
May 04, 2021
Q. Schroll
May 04, 2021
Q. Schroll
May 04, 2021
Paul Backus
May 01, 2021
Nick Treleaven
May 01, 2021
Nick Treleaven
May 02, 2021
Walter Bright
April 30, 2021
cc Walter, Atila

Phobos is like a toddler: you turn away for a few moments, and then back and something really odd has happened.

The massive duplication of declarations in Phobos is definitely unacceptable. That is, literally unacceptable - the pull requests introducing it should not have been accepted, and the issue should have been elevated as a major language issue.

I protested this before and the matter has been discussed maybe about two years ago but nothing has been done about it.

It all starts simple, like a toddler babble:

```D
alias Alias(alias a) = a;
alias Alias(T) = T;
```

The reader (including seasoned maintainer) would go like, "What? Why does it need to be defined twice? Wouldn't a type also fit an alias?" And indeed if said maintainer comments out the second declaration, a couple of obscure errors show up in unittesting (mind you, not unittesting of std.meta, but of completely unrelated modules).

Bah. Passons. Let's read further down (and skip the weird OldAlias thing, well it has package protection so it qualifies as dirty laundry), where more harbingers of bad news show up:

```D
template staticIndexOf(T, TList...)
{
    enum staticIndexOf = genericIndexOf!(T, TList);
}

template staticIndexOf(alias T, TList...)
{
    enum staticIndexOf = genericIndexOf!(T, TList);
}
```

At this point clearly there's a huge red flag waving because this is a public symbol, and if this duplication pop up at top level, it's a sign that many other public symbols will suffer the same fate. (Spoilers: they do. They all do.)


This kind of code is clearly, absolutely, pound-on-the-table, end-of-discussion not acceptable at scale. If bad code has smell, this is a rotten beached blue whale carcass.


This code should have NEVER been accepted upon review. Instead, the reviewers should have filed a TOP PRIORITY bug report to dmd "Not binding types to alias templates forces unscalable code duplication for all typelist primitives".

This needs to be fixed, and very urgently.
April 30, 2021
On 4/30/21 6:03 PM, Andrei Alexandrescu wrote:
> The massive duplication of declarations in Phobos is definitely unacceptable.

s/Phobos/std.meta/

April 30, 2021
On 4/30/21 6:05 PM, Andrei Alexandrescu wrote:
> On 4/30/21 6:03 PM, Andrei Alexandrescu wrote:
>> The massive duplication of declarations in Phobos is definitely unacceptable.
> 
> s/Phobos/std.meta/

Workaround: https://github.com/dlang/phobos/pull/8024

May 01, 2021
On Friday, 30 April 2021 at 22:03:41 UTC, Andrei Alexandrescu wrote:
> cc Walter, Atila
>
> Phobos is like a toddler: you turn away for a few moments, and then back and something really odd has happened.
>
> [...]

Ouch, didn't know about that..

A question though:
When we have fixed this, how do we make sure it doesn't keep happening?

I'm guessing more code review, but that's after the problem has already materialized.

Maybe better coding guidelines? Written by the high priest of D.

I would be very happy if we could produce some, and I would gladly read and follow them.
May 01, 2021
On 4/30/2021 3:03 PM, Andrei Alexandrescu wrote:
> ```D
> template staticIndexOf(T, TList...)
> {
>     enum staticIndexOf = genericIndexOf!(T, TList);
> }
> 
> template staticIndexOf(alias T, TList...)
> {
>     enum staticIndexOf = genericIndexOf!(T, TList);
> }
> ```

I'm curious why genericIndexOf is not renamed to staticIndexOf and the previous staticIndexOf templates removed.


> This code should have NEVER been accepted upon review. Instead, the reviewers should have filed a TOP PRIORITY bug report to dmd "Not binding types to alias templates forces unscalable code duplication for all typelist primitives".
> 
> This needs to be fixed, and very urgently.

Please file a bug report!
May 01, 2021
On Saturday, 1 May 2021 at 06:38:51 UTC, Imperatorn wrote:
> Maybe better coding guidelines? Written by the high priest of D.
>
> I would be very happy if we could produce some, and I would gladly read and follow them.

+1 (Though I don't care who writes them as long as they are accepted by DLF.)
May 01, 2021
On Friday, 30 April 2021 at 22:03:41 UTC, Andrei Alexandrescu wrote:
> the reviewers should have filed a TOP PRIORITY bug report to dmd "Not binding types to alias templates forces unscalable code duplication for all typelist primitives".

I think that did get fixed. At the time, I tried to remove the overloads in std.meta, but ran into another bug:
https://github.com/dlang/phobos/pull/7513#issuecomment-638946029

I made a dmd pull to fix that bug, but I wasn't too confident about its correctness, and the fix didn't get merged.
May 01, 2021
On Saturday, 1 May 2021 at 10:23:26 UTC, Nick Treleaven wrote:
> I made a dmd pull to fix that bug, but I wasn't too confident about its correctness, and the fix didn't get merged.

https://github.com/dlang/dmd/pull/11320


May 01, 2021
On Saturday, 1 May 2021 at 01:06:24 UTC, Andrei Alexandrescu wrote:
> On 4/30/21 6:05 PM, Andrei Alexandrescu wrote:
>> On 4/30/21 6:03 PM, Andrei Alexandrescu wrote:
>>> The massive duplication of declarations in Phobos is definitely unacceptable.
>> 
>> s/Phobos/std.meta/
>
> Workaround: https://github.com/dlang/phobos/pull/8024

More related: https://github.com/dlang/phobos/pull/8026
May 01, 2021
On 4/30/21 6:03 PM, Andrei Alexandrescu wrote:

> It all starts simple, like a toddler babble:
> 
> ```D
> alias Alias(alias a) = a;
> alias Alias(T) = T;
> ```

> 
> The reader (including seasoned maintainer) would go like, "What? Why does it need to be defined twice? Wouldn't a type also fit an alias?" And indeed if said maintainer comments out the second declaration, a couple of obscure errors show up in unittesting (mind you, not unittesting of std.meta, but of completely unrelated modules).

I'm going to take a guess that this is because older compilers would not accept keywords as alias parameters (so e.g. Alias!int would not work without the T overload). There are likely leftover workarounds, either of this type, or of the form of:

```d
template Alias(T...) if (T.length == 1)
{
   alias Alias = T[0];
}
```

I think if we can get rid of all of the overloads/workarounds, we should.

-Steve
« First   ‹ Prev
1 2 3