Skip to content
This repository was archived by the owner on Oct 12, 2022. It is now read-only.
/ druntime Public archive

Comments

Add templated _d_arraycatnTX with tests#2648

Merged
dlang-bot merged 2 commits intodlang:masterfrom
Vild:ConvertArrayCatHook
Jul 12, 2019
Merged

Add templated _d_arraycatnTX with tests#2648
dlang-bot merged 2 commits intodlang:masterfrom
Vild:ConvertArrayCatHook

Conversation

@Vild
Copy link
Contributor

@Vild Vild commented Jun 19, 2019

dmd PR: dlang/dmd#10064

This adds a templated entrypoint for _d_arraycatnTX entrypoints.
I don't think there should be any problems with this entrypoint but I do have problem with the dmd implementation.

@dlang-bot
Copy link
Contributor

dlang-bot commented Jun 19, 2019

Thanks for your pull request and interest in making D better, @Vild! 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 fetch digger
dub run digger -- build "master + druntime#2648"

@JinShil
Copy link
Contributor

JinShil commented Jun 20, 2019

Since we won't have any code utilizing this until the DMD PR is merged (but the DMD PR needs this PR), please add some unittest that directly instantiate these templates to demonstrate their functionality.

Sorry, I didn't see the unittest there, but what about _d_arraycatnTXTrace?

@@ -0,0 +1,83 @@
module rt.array.concat;

private extern (C) void[] _d_arraycatnTX(const TypeInfo ti, byte[][] arrs);
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Can this be imported instead of being declared as an extern?

Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

We should probably avoid that, IIRC druntime always uses extern declarations instead of imports for these internal functions. I guess it's to break cyclic module dependencies.

@JinShil
Copy link
Contributor

JinShil commented Jun 20, 2019

For convenience, a link to the existing runtime hook:

extern (C) void[] _d_arraycatnTX(const TypeInfo ti, byte[][] arrs)

@JinShil
Copy link
Contributor

JinShil commented Jun 20, 2019

You need to add an entry to mak/WINDOWS to get the autotester to pass. See https://github.com/dlang/druntime/pull/2643/files for an example

src/object.d Outdated
public import rt.array.comparison : __cmp;
public import rt.array.equality : __ArrayEq, __equals;
public import rt.array.casting: __ArrayCast;
public import rt.array.concat : _d_arraycatnTX, _d_arraycatnTXTrace;
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I don't like that we have these public implementation specific functions. Can the compiler look directly in rt.array.concat? Or can it be a private import and have the compiler bypass the protection?

Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

These templates are instantiated by user code, not by the compiler. The compiler just lowers expressions to them. So they need to be, source code and all, available to user programs.

Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Yes, I understand that. But currently all these functions are a weird hybrid of public API and compiler internal API.

Either these functions should be called by the user and in that case they should be well documented and have good names following the standard naming conventions. Or they should only be called by the compiler. In the latter case we should try to make them only be callable by the compiler and not by users.

Copy link
Contributor

@JinShil JinShil Jun 20, 2019

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

They can't be called by the compiler. They can only be instantiated, and then called by user code. However, the user should never instantiate and call these implementations explicitly. The user writes auto concatenatedArray = array1 ~ array2; The compiler lowers that to something like auto concatenatedArray = _d_arraycatnTX([array1, array2]); specifically so the user doesn't have to instantiate and call the template explicitly. Perhaps I'm not understanding. I don't see how else it could work.

I intend to add a upcoming PR to do something like this...

version(CoreDoc) {}
else
{
    public import rt.array.comparison : __cmp;
    public import rt.array.equality : __ArrayEq, __equals;
    public import rt.array.casting: __ArrayCast;
}

...specifically so they don't show up in the documentation.

Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

For example: _d_arraycatnTX can be private and the compiler can lower the code to:

auto concatenatedArray = __traits(getMember, rt.array, "_d_arraycatnTX")([array1, array2]);

Because __traits(getMember) can bypass the protection. Although I just noticed that it's possible to access a private function using __traits(getMember) but it's not possible to call the private function. I'm not sure if that's intentional.

Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Well, the compiler could disable access checks for these specific calls.

However, I'd simply keep things as-is for now and later on change the compiler to directly access the hooks in the original modules, not in object.d: import rt.array : _d_arraycatnTX = __some_name_which_will_never_conflict. This change might be somewhat controversial, so it's better to do this later on and not stall further progress.

@Vild Vild force-pushed the ConvertArrayCatHook branch from f34a21e to 0ea51c7 Compare June 25, 2019 17:00
@jpf91
Copy link
Contributor

jpf91 commented Jun 30, 2019

@Vild Can you adjust the windows Makefile as suggested by @JinShil to fix the Windows build? And the D_TypeInfo version as mentioned in the other PR?

After these changes, is this ready to go? Code LGTM.

@Vild Vild force-pushed the ConvertArrayCatHook branch from 0ea51c7 to 1429673 Compare June 30, 2019 23:43
@Vild
Copy link
Contributor Author

Vild commented Jun 30, 2019

There we go, I pushed the fixes.
I'm happy now with the PR.

@Vild Vild force-pushed the ConvertArrayCatHook branch from 1429673 to 8f3166d Compare July 1, 2019 17:54
@Vild
Copy link
Contributor Author

Vild commented Jul 8, 2019

This PR needs to implements the same fixes as in #2667, before it is ready to be merged

@thewilsonator
Copy link
Contributor

OK, ping me when this should be merged.

Signed-off-by: Dan Printzell <xwildn00bx@gmail.com>
@Vild Vild force-pushed the ConvertArrayCatHook branch from 8f3166d to ae86877 Compare July 9, 2019 13:39
@Vild
Copy link
Contributor Author

Vild commented Jul 9, 2019

@thewilsonator I'm done with all the changes

Sign up for free to subscribe to this conversation on GitHub. Already have an account? Sign in.

Labels

None yet

Projects

None yet

Development

Successfully merging this pull request may close these issues.

6 participants