Move some traits to druntime #2742

TurkeyMan · 2019-08-18T23:54:35Z

I need these for core.atomic...

dlang-bot · 2019-08-18T23:54:36Z

Thanks for your pull request and interest in making D better, @TurkeyMan! 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#2742"

lesderid · 2019-08-19T00:13:52Z

src/core/internal/traits.d

+}
+
+private template AliasThisTypeOf(T)
+    if (isAggregateType!T)


Shouldn't be indented.

Really? Every constraint I've seen has been intended... that's how you know it's a constraint.

Yes, "Constraints on declarations should have the same indentation level as their declaration".

that's horrible.

it's already a 50/50 split in druntime. i think it actually leans in favour of this way, which is why i went with this way.

it doesn't make sense, line-breaks always warrant an indentation.

AIUI, it's only split because the style guide was written/followed after a lot of the code had already been already written. I'm not sure I agree with this rule either, but it's required by the style guide...

Well someone can produce a PR that fixes all the code in one hit.

lesderid · 2019-08-19T00:14:05Z

src/core/internal/traits.d

+}
+
+template isFunctionPointer(T...)
+    if (T.length == 1)


lesderid · 2019-08-19T00:14:11Z

src/core/internal/traits.d

+}
+
+template isDelegate(T...)
+    if (T.length == 1)


lesderid · 2019-08-19T00:15:33Z

src/core/internal/traits.d


-template FieldTypeTuple(T)
+// TODO: deprecate these old names...?


Can't we just remove them? It's core.internal so should be fine.

That's a large project.

FieldTypeTuple is only used in this file. TypeTuple is used elsewhere in druntime too, but I can make a follow-up PR.

JinShil · 2019-08-19T00:23:57Z

If these came from Phobos, I think the unittests should be moved as well. A followup PR to Phobos to import them from druntime (and remove them from Phobos) would also be prudent IMO.

lesderid · 2019-08-19T00:29:16Z

If these came from Phobos, I think the unittests should be moved as well. A followup PR to Phobos to import them from druntime (and remove them from Phobos) would also be prudent IMO.

Other traits that were moved to druntime still have their unittests in Phobos because then they can still be used as examples (documentation isn't generated for core.internal).

TurkeyMan · 2019-08-19T00:45:09Z

A followup PR to Phobos to import them from druntime (and remove them from Phobos) would also be prudent IMO.

I have a follow-up, but it won't build until this is merged... so I haven't pushed it.

TurkeyMan · 2019-08-19T03:11:05Z

Why has buildkite gone on holiday? All PR's are not building...

lesderid · 2019-08-19T03:27:50Z

Why has buildkite gone on holiday? All PR's are not building...

Looks like the CI agent went offline, I can't ping the IP and Buildkite says 'lost connection'.

wilzbach · 2019-08-19T03:32:00Z

The CI agents are continuously spawned based on demand. Looks like there's a problem with that logic currently, but it should very likely recover over the next the hours. Not blocking the approval of this PR.

wilzbach

Their unittests should be moved too, because then Phobos can be a mere alias (and public example), but it's a bit tricky if we separate implementation from their tests.

TurkeyMan · 2019-08-19T03:41:55Z

Their unittests should be moved too, because then Phobos can be a mere alias (and public example), but it's a bit tricky if we separate implementation from their tests.

Documented tests stay in phobos, and the tests are documented tests.
We do this for all the other tests.

We would need to do a pass to move all the docs over to druntime, which would mean moving core.internal.traits to core.traits... that's way outside the scope of this PR, I'm not changing any organisation here.

JinShil · 2019-08-19T03:48:40Z

I think only those unittests with a /// DDoc header are used for documentation (@wilzbach, please correct me if I'm wrong). Can we at least move the unittests which are not used for documentation? For example: https://github.com/dlang/phobos/blob/81572ffce650cb17319c63927cffa6f1e345a094/std/traits.d#L7543-L7555

TurkeyMan · 2019-08-19T03:51:26Z

@JinShil That is duplicated in druntime and phobos (because not public)... are you saying the unit test should exist in both places?

JinShil · 2019-08-19T03:58:57Z

are you saying the unit test should exist in both places?

With regard to ModifyTypePreservingTQ, it is package. and since these traits are being moved to core.internal, I would expect the entire thing (unittests and implementation) to be moved to druntime and made public. Then it can be package-imported in Phobos. e.g.

package import core.internal : ModifyTypePreservingTQ;

I don't know if package import is a thing though; I'll have to try it out.

Edit: However, my prior comment was more general. I suggest moving the unittests that are not documentation unittests to druntime where the implementation is.

JinShil · 2019-08-19T04:02:08Z

I don't know if package import is a thing though; I'll have to try it out.

Even if package import is not legal, you can still make ModifyTypePreservingTQ public in druntime and then make a package wrapper like you did with some of the other imports in dlang/phobos#7148

TurkeyMan · 2019-08-19T05:36:05Z

Oh FFS!! See what happens! I move the tests over and they depend on 90% of phobos...
This happens every time! That's why I don't do it.

This is blocking me, one of you can fix this. Please be fast...

TurkeyMan · 2019-08-19T05:37:00Z

I want to start getting paid for the time wasted by bullshit like this. I have finite lifetime, and it's worth $100/hr.

TurkeyMan · 2019-08-20T06:38:46Z

Should merge this; everyone got bent-out-of-shape about the tests... and from here I can push the phobos PR...

TurkeyMan · 2019-08-23T06:03:59Z

Surely this is trivial?

Unittests added

TurkeyMan requested review from MartinNowak and schveiguy as code owners August 18, 2019 23:54

thewilsonator approved these changes Aug 18, 2019

View reviewed changes

lesderid approved these changes Aug 19, 2019

View reviewed changes

TurkeyMan mentioned this pull request Aug 19, 2019

Alias traits from druntime dlang/phobos#7148

Merged

wilzbach previously requested changes Aug 19, 2019

View reviewed changes

TurkeyMan force-pushed the move_some_trsits branch from d388a08 to a40b60c Compare August 19, 2019 04:27

TurkeyMan force-pushed the move_some_trsits branch 3 times, most recently from 55cba5e to 8f0cfe2 Compare August 20, 2019 02:55

JinShil approved these changes Aug 20, 2019

View reviewed changes

Move unit-test, since the last patch was merged with the other branch...

52894b5

TurkeyMan force-pushed the move_some_trsits branch from 8f0cfe2 to 52894b5 Compare August 22, 2019 02:51

thewilsonator approved these changes Aug 23, 2019

View reviewed changes

thewilsonator added the auto-merge label Aug 23, 2019

dlang-bot merged commit 396a0ec into dlang:master Aug 23, 2019

TurkeyMan deleted the move_some_trsits branch August 23, 2019 07:45

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Move some traits to druntime #2742

Move some traits to druntime #2742

TurkeyMan commented Aug 18, 2019

dlang-bot commented Aug 18, 2019

lesderid Aug 19, 2019

TurkeyMan Aug 19, 2019

lesderid Aug 19, 2019

TurkeyMan Aug 19, 2019 •

edited

Loading

lesderid Aug 19, 2019

TurkeyMan Aug 19, 2019

lesderid Aug 19, 2019

lesderid Aug 19, 2019

lesderid Aug 19, 2019

TurkeyMan Aug 19, 2019

lesderid Aug 19, 2019

TurkeyMan Aug 19, 2019

JinShil commented Aug 19, 2019

lesderid commented Aug 19, 2019

TurkeyMan commented Aug 19, 2019

TurkeyMan commented Aug 19, 2019

lesderid commented Aug 19, 2019

wilzbach commented Aug 19, 2019

wilzbach left a comment

TurkeyMan commented Aug 19, 2019

JinShil commented Aug 19, 2019

TurkeyMan commented Aug 19, 2019

JinShil commented Aug 19, 2019 •

edited

Loading

JinShil commented Aug 19, 2019 •

edited

Loading

TurkeyMan commented Aug 19, 2019

TurkeyMan commented Aug 19, 2019

TurkeyMan commented Aug 20, 2019

TurkeyMan commented Aug 23, 2019


		template FieldTypeTuple(T)
		// TODO: deprecate these old names...?

Move some traits to druntime #2742

Move some traits to druntime #2742

Conversation

TurkeyMan commented Aug 18, 2019

dlang-bot commented Aug 18, 2019

Bugzilla references

Testing this PR locally

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

TurkeyMan Aug 19, 2019 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

JinShil commented Aug 19, 2019

lesderid commented Aug 19, 2019

TurkeyMan commented Aug 19, 2019

TurkeyMan commented Aug 19, 2019

lesderid commented Aug 19, 2019

wilzbach commented Aug 19, 2019

wilzbach left a comment

Choose a reason for hiding this comment

TurkeyMan commented Aug 19, 2019

JinShil commented Aug 19, 2019

TurkeyMan commented Aug 19, 2019

JinShil commented Aug 19, 2019 • edited Loading

JinShil commented Aug 19, 2019 • edited Loading

TurkeyMan commented Aug 19, 2019

TurkeyMan commented Aug 19, 2019

TurkeyMan commented Aug 20, 2019

TurkeyMan commented Aug 23, 2019

TurkeyMan Aug 19, 2019 •

edited

Loading

JinShil commented Aug 19, 2019 •

edited

Loading

JinShil commented Aug 19, 2019 •

edited

Loading