introduce root.Array.only and test it out in expressionsem

[benjones]

There's a lot of places where we create an empty array (often new Objects();) and then push 1 or 2 items into it.

This PR adds a method like phobos's only to create these arrays more succinctly

I considered using typesafe variadics, but IIRC we're trying to move away from those so I used a scope slice instead (should it be return scope?)

If folks like this I'll grep for new \w+s\(\) to find other array creation spots and do similar replacements as I did in expressionsem

[dlang-bot]

Thanks for your pull request and interest in making D better, @benjones! We are looking forward to reviewing it, and you should be hearing from a maintainer soon.
Please verify that your PR follows this checklist:

• My PR is fully covered with tests (you can see the coverage diff by visiting the details link of the codecov check)
• My PR is as minimal as possible (smaller, focused PRs are easier to review than big ones)
• I have provided a detailed rationale explaining my changes
• New or modified functions have Ddoc comments (with Params: and Returns:)

Please see CONTRIBUTING.md for more information.

---

If you have addressed all reviews or aren't sure how to proceed, don't hesitate to ping us with a simple comment.

Bugzilla references

Your 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 locally

If you don't have a local development environment setup, you can use Digger to test this PR:

dub run digger -- build "master + dmd#21520"

[ibuclaw]

I wouldn't object to

this(T[]...)

Otherwise, variadic templates aren't going anywhere, could have a top-level template. The name only looks a bit odd for a construction name, so I vote to call it make

[Edited, just tested]

struct Array(T)
{
...
}

auto make(T, E...)(E elems)
if (is(T E == Array!E))
{
    auto ret = new T();
    foreach (e; elems)
    {
        ret.push(e);
    }
    return ret;
}

Usage

auto tiargs = make!Objects(e1, e2);

You'll have to foreach this way because bootstrap compiler doesn't understand static foreach, and ctfe isn't smart enough to unroll 0..length.

[benjones]

I named it only to match std.range.only but I agree it's a bit weird here. Happy to rename or make it a constructor or standalone template based on what folks like best. I'd add a ret.reserve(elems.length) to what you had, but otherwise that looks fine to me.

I originally tried to do slice assign (ret[] = E[]) and it said that ret[] wasn't an lvalue, so I guess I could add a ref overload of opSlice if that seems useful 🤷

Anyone else have thoughts on the nicest approach?

[dkorpel]

I like the idea of replacing those calls to push with a single call, and I don't want to bikeshed so I'm fine with this PR either way, but I wouldn't call it only because the fact that is matches a Phobos name only confuses me. I see only as a range interface wrapper over a static array. This PR's function is not used for ranges, but simply to conveniently construct an array, which is what a constructor is for.

[benjones]

Sounds good, I changed it to a typesafe variadic constructor and that feels right to me

[benjones]

Ah, when T is int, with a single argument, it's ambiguous for constructor taking the initial size. Thoughts?

[ibuclaw]

Ah, when T is int, with a single argument, it's ambiguous for constructor taking the initial size. Thoughts?

For now, wrap it around static if (is(T == struct) || is(T == class)) or whatever equivalent that blocks out integers.

Later it may become obvious that we don't need the (size_t dim) constructor.

[benjones]

Failing when testing on version 2.095.0 from 2021, which takes issue with the static if. I make the constructor a no-arg template and change that to a template constraint? Is there a schedule/list of expected bootstrap version cutoffs?

[ibuclaw]

Interesting. gdc-9 doesn't choke on this, and that's 2.076 based. Ideally there should be no breaking bootstrap using previous releases. You can switch to stable to have CI run on all from 2.076 onwards to find out when it got broke and fixed - not that we can do anything about it other than add a note / targetted workaround.

[benjones]

Hmm, ok. It's especially weird because the error doesn't even make sense:

/home/circleci/dmd/compiler/src/dmd/root/array.d(59): Error: `static if` conditional cannot be at global scope static if (is(T == struct) || is(T == class))

... it's not. It's at struct scope

[ibuclaw]

I was about to say I cannot reproduce it when I looked again at CI.

(DMD) CXX-HEADERS

Ohhh. C++ header generation. We don't care about backwards compat for that. :-)

[ibuclaw]

@benjones feel free to bump circleci to 2.098.0

[ibuclaw]

OK, apologies on my end. I thought I cleared the build directory before testing.

Looks like this is a bug in dtoh also in mainline.

extern(C++) struct Array(T)
{
    static if (true)
    {
    }
}
// $ ./generated/linux/release/64/dmd test.d -HC
// test.d(3): Error: `static if` conditional cannot be at global scope

[benjones]

Huh, hopefully my dummy template hack fixes this and I'll look at the dtoh bug later if I get a chance

[ibuclaw]

C++ has no equivalent, so can only improve the error message if dtoh hits a static if within a template declaration.

See ignored() because it's inside of a template declaration.

[ibuclaw]

Only nits, but otherwise fine.