Don't needlessly import everything in object.d when compiling with -betterC

[JinShil]

This is a subset of #2184 to remove unnecessary imports from object.d when compiling with -betterC, yet without the controversy surrounding nothrow attribution in -betterC.

Although -betterC does not link with druntime or phobos, it still imports object.d. There is a large amount of code in object.d that not utilized when compiling with -betterC, but it is not versioned out like it should be, and that causes the compiler to emit errors for code that isn't even being utilized under certain circumstances.

It is likely future PRs will need to be made to hoist certain templates out of the version(Not_BetterC) area up to the version(D_BetterC)` area as users utilize -betterC more and file issues identifying certain language features that don't work in a -betterC environment. It will also be beneficial to acquire those issue reports to identify lapses in test coverage for -betterC.

This is a prerequisite for #2184 and dlang/dmd#8253, but is still valuable even if those PRs are never merged.

cc @WalterBright

[dlang-bot]

Thanks for your pull request, @JinShil!

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 fetch digger
dub run digger -- build "master + druntime#2194"

[jacob-carlborg]

Using Objective-C together with betterC should be possible.

[JinShil]

Done. Thanks!

[JinShil]

FYI: This is just a code rearrangement PR. The only new code is https://github.com/JinShil/druntime/blob/fd51ed7f77cba5afe8edeece6631691b1bd83f58/src/object.d#L135-L144

[jacob-carlborg]

Is this testable? Compile something with betterC, try to access something that shouldn’t be accessible and verify that it fails?

[n8sh]

Why should these templates be removed if compiling D_betterC?

// compiler frontend lowers dynamic array comparison to this
bool __ArrayEq(T1, T2)(T1[] a, T2[] b)
{
    if (a.length != b.length)
        return false;
    foreach (size_t i; 0 .. a.length)
    {
        if (a[i] != b[i])
            return false;
    }
    return true;
}

// compiler frontend lowers struct array postblitting to this
void __ArrayPostblit(T)(T[] a)
{
    foreach (ref T e; a)
        e.__xpostblit();
}

// compiler frontend lowers dynamic array deconstruction to this
void __ArrayDtor(T)(T[] a)
{
    foreach_reverse (ref T e; a)
        e.__xdtor();
}

Same question for these:

// lhs == rhs lowers to __equals(lhs, rhs) for dynamic arrays
bool __equals(T1, T2)(T1[] lhs, T2[] rhs);

int __cmp(T)(const T[] lhs, const T[] rhs) @trusted
if (__traits(isScalar, T));

// This function is called by the compiler when dealing with array
// comparisons in the semantic analysis phase of CmpExp. The ordering
// comparison is lowered to a call to this template.
int __cmp(T1, T2)(T1[] s1, T2[] s2)
if (!__traits(isScalar, T1) && !__traits(isScalar, T2));

// Compiler hook into the runtime implementation of array (vector) operations.
template _arrayOp(Args...)
{
    import core.internal.arrayop;
    alias _arrayOp = arrayOp!Args;
}

/*
 * Support for switch statements switching on strings.
 * Params:
 *      caseLabels = sorted array of strings generated by compiler. Note the
                   strings are sorted by length first, and then lexicographically.
 *      condition = string to look up in table
 * Returns:
 *      index of match in caseLabels, a negative integer if not found
*/
int __switch(T, caseLabels...)(/*in*/ const scope T[] condition) pure nothrow @safe @nogc;

Same question here. I believe that core.internal.hash: hashOf doesn't use TypeInfo or anything from the rt.<whatever> modules.

size_t hashOf(T)(auto ref T arg, size_t seed = 0)
{
    import core.internal.hash;
    return core.internal.hash.hashOf(arg, seed);
}

[JinShil]

Why should these templates be removed if compiling D_betterC?

Dynamic arrays cannot be supported in -betterC code because they require the GC.

I think hashOf and __switch could be hoisted into the common area, but I need some test cases for that so I can add them to DMD. If they are valid in -betterC they should already be added to the DMD test suite regardless of the fate of the PR.

[JinShil]

Here's a coding challenge for reviewers: Please help me break this PR by supplying test cases that work today in -betterC, but won't work if this PR is merged. I can then add them to DMD to break this PR, and then adjust this PR to make them pass.

[n8sh]

Dynamic arrays cannot be supported in -betterC code because they require the GC.

For the purpose of array comparison, I would assume that the comment about "dynamic arrays" really means "array slices".

[JinShil]

Is this testable? Compile something with betterC, try to access something that shouldn’t be accessible and verify that it fails?

Good question. It took me a while to think this through, but basically this code:

extern(C) void main() 
{
    Object o = new Object();
}

main.o: In function `main':
main.d:(.text.main[main]+0x7): undefined reference to `_D6Object7__ClassZ'
main.d:(.text.main[main]+0xc): undefined reference to `_d_newclass'
collect2: error: ld returned 1 exit status
Error: linker exited with status 1

...compiles now because Object is being imported , but won't link because druntime is not linked. It's basically nonsense to write code like this in -betterC, so there should be a compiler error. With this PR the compiler will generate

main.d(3): Error: undefined identifier Object, did you mean import object?

So, yeah, we can test this. Perhaps I'll add a basic test to this PR for druntime, and to demonstrate this PR's implementation, but I'd rather put the bulk of tests in DMD.

[JinShil]

For the purpose of array comparison, I would assume that the comment about "dynamic arrays" really means "array slices".

The following code still works with this PR in -betterC (array slices of static arrays).

extern(C) void main() 
{
    int[6] a1 = [1,2,3,4,5,6];
    int[3] a2 = a1[0..3];
    int[3] a3 = a1[3..$];

    assert(a2 == a1[0..3]);
    assert(a3 == a1[3..$]);
}

So I think the current state of this PR with respect to __ArrayEq and friends is correct.

[JinShil]

__equal, __cmp, and __switch need to be hoisted. I added test coverage at dlang/dmd#8308. That PR should be pulled before this.

[JinShil]

I tested hashOf in the current impementation of -betterC, but it doesn't work.

extern(C) void main() 
{
    int a;
    int b;
    assert(hashOf(a) == hashOf(b));  // Error: TypeInfo cannot be used with -betterC
}

I have no idea why TypeInfo is required there, but I can't solve all of these -betterC problems at once. I need to do this in pieces, so I need these PRs pulled so I can move on to the next steps.

[JinShil]

dlang/dmd#8308 was merged, which adds testing for __equals and __cmp when compiling with -betterC. This caused expected CI failures:

• https://auto-tester.puremagic.com/show-run.ghtml?projectid=1&runid=3185309&isPull=true
• https://auto-tester.puremagic.com/show-run.ghtml?projectid=1&runid=3185343&isPull=true

I've, therefore, hoisted __cmp and __equals into the betterC section of object.d, and now this PR is passing the CIs.

I've discovered a number of problems with the implementation of -betterC in this process. For example, string comparisons require a runtime call to dstrcmp which causes link-time failures in both the current -betterC implementation and the implementation of this PR. Also, calls to hashOf require TypeInfo in both the current implementation and the implementation in this PR. There are others.

I'm confident all such things can be fixed, but I can't do it all in this PR. I need this PR merged first so I have some infrastructure to work off of.

I've decided against adding a failing test for something like...

extern(C) void main() 
{
    Object o = new Object();
}

... when compiling with -betterC. The reason why, is we don't have good testing infrastructure for druntime yet. We need the unittests in druntime to run twice, once with -betterC, and once without. Then I can add version(D_BetterC) conditional compilations to the unittests and we can start adding -betterC tests that way. That, however, will require a separate PR to the makefiles to put that infrastructure in place.

From my perspective this is ready for additional scrutiny and hopefully a merge. But it is just the beginning.

[dnadlinger]

Regarding arrays, that's because dlang/dmd#8067 has not been merged yet.

[JinShil]

dlang/dmd#8067 was merged. As a result, I was able to remove _ArrayEq as it appears to have been replaced in that PR by __ArrayEq (notice the extra underscore).

Again, this is ready to go unless someone can identify a test case that this PR breaks.

[PetarKirov]

I recommend moving the whole overload sets of destroy, __cmp, etc. to the top and simply wrapping the ones that need druntime to be linked in to be wrapped with version(Not_BetterC), in order to keep the code clean, easy to understand and not to break the documentation (AFAIK, DDoc expect /// ditto-ed overloads to follow lexically the overloads which carry the main documentation comment).

[JinShil]

@ZombineDev You are right, and I implemented that, but now look at the diff; it's a monstrosity. Reviewers will probably find it easier to just review the resulting file, without the diff.

I'm thinking maybe we should start breaking object.d into smaller files and publicly import them into object.d.

[PetarKirov]

@ZombineDev You are right, and I implemented that, but now look at the diff; it's a monstrosity. Reviewers will probably find it easier to just review the resulting file, without the diff.

Perhaps splitting each moved function in a separate commit can help with that.

I'm thinking maybe we should start breaking object.d into smaller files and publicly import them into object.d.

I'm very much in favor of this. I would propose splitting it in core/internal/lifetime (for destroy related functionality), core/internal/array (for dynamic and associative array support), core/internal/builtin (for compiler support functions like __cmp).
However I would strongly suggest we get support from the other druntime contributors before in order to avoid last minute rejection.

[jacob-carlborg]

I like splitting it up in multiple files as well, but I Andrei has been against it in the past.

[JinShil]

I don't want to do a lot of refactoring in this. I just want to prevent -betterC compilations from importing code it will never use. Is this PR currently not reviewable? What do I need to do to get a review?

[jacob-carlborg]

Is this PR currently not reviewable? What do I need to do to get a review?

I've already approved it, but I guess there's been additional changes after that?

[JinShil]

Yes, based on feedback from others, I've made a few changes. I just need to know what I need to do to get approval.

EDIT: Basically what I did was hoist __cmp, __equals, and destroy implementations to the top of the file, and version out the ones that are not needed for -betterC builds. I also deleted _ArrayEq because it was replaced by __ArrayEq in dlang/dmd#8067.

[jacob-carlborg]

Sounds fine to me, but it's quite difficult to verify now.

[JinShil]

Sounds fine to me, but it's quite difficult to verify now.

That's why I recommended just viewing the resulting file. I don't think there's anything I can do about the diff. Even if I split them into different commits, they will be accumulative so maybe only the first will be comprehendible. Maybe I could split it into different PRs. Would that be better?

[jacob-carlborg]

That's why I recommended just viewing the resulting file

I think that's the easiest.

[JinShil]

Thanks Jacob!

[WalterBright]

With 3000 lines of changes, this PR is essentially not reviewable.

[JinShil]

There wasn't 3000 lines of changes; that was just what the diff reported due to shortcomings in github's diff algorithm. The top 850 lines of code is where all the changes are, and the the vast majority of that is just copy and paste from locations further down in the file. Those monitoring the evolution of this PR were able to understand what was being changed and why, and were able to review the resulting file.