Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Simpler emplace #3805

Merged
merged 5 commits into from
Nov 14, 2015
Merged

Simpler emplace #3805

merged 5 commits into from
Nov 14, 2015

Conversation

andralex
Copy link
Member

The logic in emplace copies the logic used by the compiler itself in generating constructors. After sleeping on it for a night I figured I could get most of that logic just work for us by defining a struct with only one member and one constructor. Then the rest is taken care of.

@adamdruppe
Copy link
Contributor

Looks like a commit about static assert got mixed in this branch too....

@andralex
Copy link
Member Author

Boy I now messet stuff up... hold on...

@brad-anderson
Copy link
Member

Where does the name kakados come from? Just curious.

@andralex
Copy link
Member Author

I knew it!

That code was in an experimental state. When I introduce experimental names, I use names that would never be plausible in the production codebase, like crap or worse. "Caca" means "poop" in Romanian. "Cacados" would be a pretend-Spanish version of it (that doesn't actually exist but sounds Spanish), same as e.g. "los poopos" in English. Now to have additional fun, I also spelled it with "k"s instead of "c"s.

Happened to me in the Facebook codebase... and was seen by my Romanian coworkers :o).

@andralex
Copy link
Member Author

Alright I guess I fixed it but will need to wait for #3804.

@brad-anderson
Copy link
Member

"Caca" means poop in English too :)

@andralex
Copy link
Member Author

@eco I live and I learn

@JakobOvrum
Copy link
Member

Looking good! Thanks for these changes, I never understood why emplace[Ref] were so messy. I've encountered a fair number of unhandled edge cases with it, but since piling on more spaghetti code isn't scaleable I just shrugged and moved on. Hopefully we can make it more general.

@andralex
Copy link
Member Author

BTW this adds documentation examples. But I'm still unhappy about dox - I think all overloads should appear in one place. Will get to that later.

@andralex
Copy link
Member Author

tests passed :)

static if (!is(T == struct))
void emplaceImpl(Arg)(ref UT chunk, auto ref Arg arg)
else static if (
!is(T == struct) && Args.length == 1 /* primitives, enums, arrays */
Copy link
Contributor

Choose a reason for hiding this comment

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

I think this needs to check for unions, too.

Copy link
Member Author

Choose a reason for hiding this comment

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

Yah, it occurred to me too. That would be an enhancement. I've added a test case for unions (see my next commit) and they seem to work. Could you please post a few tests that fail?

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 have any. And thinking about it, unions can have neither postblits nor destructors (and can't contain members that do), so they're POD types. They therefore shouldn't need special treatment.

Copy link
Member Author

Choose a reason for hiding this comment

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

guess we're ready for the mergearoo!

@WalterBright
Copy link
Member

How's it doing on:

dmd std/conv.d -unittest -cov -main

?

@andralex
Copy link
Member Author

@WalterBright looks good, only CTFE code and code with assert(0) appears uncovered. BTW we shouldn't count paths leading unconditionally to assert(0) as uncovered.

I pushed one more commit to nominally improve coverage.

@andralex
Copy link
Member Author

WalterBright added a commit that referenced this pull request Nov 14, 2015
@WalterBright WalterBright merged commit 48d57e3 into dlang:master Nov 14, 2015
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

6 participants