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

Fix appender form elaborate assign types #1529

Merged
merged 3 commits into from Oct 27, 2013
Merged

Fix appender form elaborate assign types #1529

merged 3 commits into from Oct 27, 2013

Conversation

monarchdodra
Copy link
Collaborator

Follow up on emplace fixes: This makes use of emplace in appender, as one should not be doing naked assigns on non-initialized memory.

Fixes:
http://d.puremagic.com/issues/show_bug.cgi?id=10690
http://d.puremagic.com/issues/show_bug.cgi?id=10859

Also, fixed a little issue with strong exception safety: The appender _data was being modified to reference the new items, before they were actually constructed. This left appender in an unspecified state if an exception is thrown.

Also, used named functions, to tone down on a bit on lambda useage (more readable too, IMO), as well as mitigate performance issues until we get the inline lambda fix/feature.

Assigning to @klickverbot . Pinging @9rnsr .

@monarchdodra monarchdodra mentioned this pull request Aug 28, 2013
@monarchdodra
Copy link
Collaborator Author

About the implementation fix: The code I used is "proven and tested" in std.array.array. It was copied as-is, and only updated for the arguments.

@monarchdodra
Copy link
Collaborator Author

Oops. I left in a little bug. I fixed it, but the code isn't optimal anymore. I'll work on it.

@monarchdodra
Copy link
Collaborator Author

Done. This should be more straight forward now. I also added some extra unittests. This also fixes "Issue 9528 - std.array.appender can't append elements with const members".
http://d.puremagic.com/issues/show_bug.cgi?id=9528

This is sweet actually, as it also fixes the duplicate "Issue 10753 - std.array.array of a range of structs with immutable fields" too, which is another feature users asked us for.

@quickfur
Copy link
Member

Sweet! This is a much anticipated and much needed fix.

@dnadlinger
Copy link
Member

To me, it seems like the whole emplace issue is getting a bit out of hand – if we need a ton of special cases in emplace itself as well as every single call site, we should step back and ask ourselves if the abstractions we chose are the right ones.

As for this specific pull request, I probably won't find the time to properly review the implementation in the next few weeks. The tests seem sensible, but I'd prefer if somebody looked at the implementation in detail before merging.

@monarchdodra
Copy link
Collaborator Author

To me, it seems like the whole emplace issue is getting a bit out of hand

It's historic actually. Calling emplace means getting a pointer, and getting a pointer is unsafe. That, and emplace was originally bugged enough that we would avoid using it if we could. But there is no actual reason to do so, and I think I will stop doing it now. It's gratuitous and complicates the code for nothing.

if we need a ton of special cases in emplace

The special cases are actually for constructing something from "given args". The common case, where we basically want to emplace a copy is actually not that complicated.

I'll admit things would have been much simpler if we had differently named functions, instead of trying to guess which overload should be called. But that ship has passed (mostly).

@monarchdodra
Copy link
Collaborator Author

@klickverbot : I deployed a conditional-less emplace in array. Give it a quick review? If it is fine, I'll do the same here. This is indeed gratuitously complicated.

static if (is(Unqual!T == T))
alias uitem = item;
else
auto ref uitem() @trusted nothrow @property { return cast(Unqual!T)item;}
Copy link

Choose a reason for hiding this comment

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

Hmm, auto ref? I don't think ref can work with a cast, e.g.:

float y;
ref int x() { return cast(int)y; }  // Error: cast(int)y is not an lvalue

Copy link
Member

Choose a reason for hiding this comment

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

yah that's never going to yield a ref

@ghost
Copy link

ghost commented Oct 23, 2013

Apart from my comments this is a nice cleanup. The old syntax was really giving me a headache, I mean stuff like this:

()@trusted{ return _data.arr.ptr[i .. i + 1]; }()[0]
=
()@trusted{ return cast(Unqual!T)items.front; }();

This would make anyone's head spin.

@WalterBright
Copy link
Member

I'm not sure - is this ready to merge or not?

()@trusted{ return cast(Unqual!T)item; } ();
()@trusted{ _data.arr = _data.arr.ptr[0 .. len + 1]; }();

auto bigDataFun() @trusted nothrow { return _data.arr.ptr[0 .. len + 1];}
Copy link
Member

Choose a reason for hiding this comment

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

yah it's a bummer we need to resort to such tricks...

@andralex
Copy link
Member

Looks like this could be improved but I'll merge now in the interest of time. Further improvements welcome.

andralex added a commit that referenced this pull request Oct 27, 2013
Fix appender form elaborate assign types
@andralex andralex merged commit f9712aa into dlang:master Oct 27, 2013
@monarchdodra
Copy link
Collaborator Author

Looks like this could be improved but I'll merge now in the interest of time. Further improvements welcome.

I thought this was "urgency" of the merge was to get this into 2.064?
https://d.puremagic.com/issues/show_bug.cgi?id=11474

Well, I still have some cleaning up to do anyways.

static if (mustEmplace)
emplace(&it, getUItem(items.front));
else
it = getUItem(items.front);
Copy link
Collaborator Author

Choose a reason for hiding this comment

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

Interesting fact: There is no items.popFront in this loop.

We are, in fact, repeatedly copying the first item :/

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