Skip to content
This repository has been archived by the owner on Oct 12, 2022. It is now read-only.

Refactor object.d: Move __cmp and __equals to rt #2634

Merged
merged 1 commit into from Jun 17, 2019
Merged

Refactor object.d: Move __cmp and __equals to rt #2634

merged 1 commit into from Jun 17, 2019

Conversation

JinShil
Copy link
Contributor

@JinShil JinShil commented Jun 6, 2019

This is an exploration into ideas for refactoring object.d so it's not a dumping ground for everything.

This approach utilizes packages and modules more than has been typically done in other D modules in an attempt to leverage D's encapsulation features and isolate things that don't need each other to their own modules.

To make templated lowerings available to the compiler, they are publicly imported into object.d. I'm not sure how that's going to turn out, hence the draft PR. Those modules that have templated lowerings are copied to the import directory so they are available to object.d.

cc @Vild @jpf91

@jacob-carlborg
Copy link
Contributor

I think Andrei has been against this in the past due to the compiler needs to read more files.

@thewilsonator
Copy link
Contributor

Whatever happened to reading zip archives of "precompiled" packages?

@JinShil
Copy link
Contributor Author

JinShil commented Jun 6, 2019

I think Andrei has been against this in the past due to the compiler needs to read more files.

And what's the alternative? Put the entire runtime into object.d? Because that's where we're headed.

@JinShil
Copy link
Contributor Author

JinShil commented Jun 6, 2019

Whatever happened to reading zip archives of "precompiled" packages?

I don't know, that's the first I've heard of it. Anyway these files contain templates, so I'm not sure what precompilation can do.

@jacob-carlborg
Copy link
Contributor

jacob-carlborg commented Jun 6, 2019

And what's the alternative? Put the entire runtime into object.d? Because that's where we're headed.

I’m in favor of this PR. Just pointing out that there’s been talk about this before.

@JinShil
Copy link
Contributor Author

JinShil commented Jun 9, 2019

Can anyone tell me what's failing in the Circle CI and AutoTester CIs? I don't see what the problem is.

@jacob-carlborg
Copy link
Contributor

CircleCI seems to be failing due to:

diff mytrace.def.exp ./generated/linux/debug/64/mytrace.def || diff mytrace.releaseopt.def.exp ./generated/linux/debug/64/mytrace.def
3d2
< 	_D4core8internal6string__T7dstrcmpZQjFNaNbNiNeMxAaMxQeZi
5a5
> 	_D4core8internal6string__T7dstrcmpZQjFNaNbNiNeMxAaMxQeZi
3d2
< 	_D4core8internal6string__T7dstrcmpZQjFNaNbNiNeMxAaMxQeZi
5a5
> 	_D4core8internal6string__T7dstrcmpZQjFNaNbNiNeMxAaMxQeZi

@jacob-carlborg
Copy link
Contributor

The test failures might be related to the changes in test/profile.

@JinShil
Copy link
Contributor Author

JinShil commented Jun 9, 2019

CircleCI seems to be failing due to:

Yes, but I don't see any difference in that diff, and I can't reproduce it locally.

@JinShil
Copy link
Contributor Author

JinShil commented Jun 9, 2019

The test failures might be related to the changes in test/profile.

The changes in test/profile are an attempt to resolve the diff. Look at the diff output. What's different?

@jacob-carlborg
Copy link
Contributor

I don't see a difference. Whitespace or no printable character?

@JinShil
Copy link
Contributor Author

JinShil commented Jun 10, 2019

Whitespace or no printable character?

I checked whitespace and EOL characters, and everything seems correct. The indent is a tab, but I doublechecked that the files also used a tab character. @wilzbach @thewilsonator Any ideas?

@thewilsonator
Copy link
Contributor

Dunno. All the results are old though.

@JinShil
Copy link
Contributor Author

JinShil commented Jun 11, 2019

@rainers I'm having a lot of trouble getting this PR to pass the profile test suite and I'm hoping you may have some idea.

The problem is the list of exports in test/profile/mytrace.def.exp. For debug builds, the test suite passes with my changes. For release builds, it does not. It seems _D2rt5array10comparison__T5__cmpTaZQjFNaNbNiNeMxAaMxQeZi appears in the list of exports for debug builds, but does not appear in the list of exports for release builds.

I don't know much about what the profile code is doing and why these .exp files even exist. Do you have any knowledge of this that you would be willing to share to help me resolve this problem?

@rainers
Copy link
Member

rainers commented Jun 11, 2019

@JinShil I'm not too familiar with that code, but it seems to me that the makefile allows either mytrace.def.exp or mytrace.releaseopt.def.exp to match the generated file, because the call to __cmp isn't generated in the release build (no code for asserts).

I don't know what your previous attempts did, but I guess you should just not add _D2rt5array10comparison__T5__cmpTaZQjFNaNbNiNeMxAaMxQeZi to mytrace.releaseopt.def.exp.

@n8sh
Copy link
Member

n8sh commented Jun 11, 2019

How can template code be in rt.*, since the idea is that it is compiled separately?

@JinShil
Copy link
Contributor Author

JinShil commented Jun 11, 2019

How can template code be in rt.*, since the idea is that it is compiled separately?

rt is intended to be the compiler<-->runtime interface. It is the implementation of the language, and that is precisely what the code in these new files are. The templates in this PR are not intended to be called directly by the user, but they need to be automatically imported and publicly available to user code to make the compiler lowerings work.

object.d is definitely not the right place for these things, and it is growing into a monstrosity with more on the way due to this GSoC project. Furthermore, when implementations are added to object.d they often need to come up with clever tricks to peek into rt in order to utilize some of the implementations there. With this PR the implementations stay in rt.

@JinShil
Copy link
Contributor Author

JinShil commented Jun 11, 2019

because the call to __cmp isn't generated in the release build (no code for asserts).

Everything immediately made sense after reading that. Thanks! @rainers

@JinShil JinShil marked this pull request as ready for review June 11, 2019 09:54
Copy link
Member

@wilzbach wilzbach left a comment

Choose a reason for hiding this comment

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

I'm in favor of this too. Reading files is cheap, evaluating templates isn't. And if reading files ever becomes an overhead, we can always bundle everything during the release build into one file.

@dlang-bot
Copy link
Contributor

Thanks for your pull request and interest in making D better, @JinShil! 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 annotated coverage diff directly on GitHub with CodeCov's browser extension
  • 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#2634"

@n8sh
Copy link
Member

n8sh commented Jun 12, 2019

Do templates get evaluated even when they're not instantiated?

@jacob-carlborg
Copy link
Contributor

Do templates get evaluated even when they're not instantiated?

No semantic analysis is run for templates unless they’re instantiated.

@thewilsonator thewilsonator added the 72h no objection -> merge The PR will be merged if there are no objections raised. label Jun 13, 2019
@JinShil
Copy link
Contributor Author

JinShil commented Jun 16, 2019

ping

Sign up for free to subscribe to this conversation on GitHub. Already have an account? Sign in.
Labels
None yet
Projects
None yet
7 participants