EricWF resigned from this revision.
EricWF removed a reviewer: EricWF.
EricWF added a comment.
This revision now requires review to proceed.
This is done.
http://reviews.llvm.org/D16360
___
cfe-commits mailing list
cfe-commits@lists.llvm.org
http://
r263746.
> On 2016-Mar-17, at 13:36, Duncan P. N. Exon Smith via cfe-commits
> wrote:
>
>>
>> On 2016-Mar-16, at 15:41, Eric Fiselier wrote:
>>
>> EricWF accepted this revision.
>> EricWF added a comment.
>> This revision is now accepted and ready to land.
>>
>> LGTM after change in inline
> On 2016-Mar-16, at 12:31, Duncan P. N. Exon Smith
> wrote:
>
>>
>> On 2016-Mar-16, at 12:20, Eric Fiselier wrote:
>>
>> EricWF added a comment.
>>
>> Adding inline comments for the implementation. Comments on the tests to
>> follow shortly.
>>
>>
>>
>> Comment at: incl
EricWF added a comment.
Adding inline comments for the implementation. Comments on the tests to follow
shortly.
Comment at: include/__hash_table:103
@@ -102,1 +102,3 @@
+template
+struct __extract_key;
Could you make `__extract_key` behave the same way as `_
> On 2016-Mar-16, at 15:41, Eric Fiselier wrote:
>
> EricWF accepted this revision.
> EricWF added a comment.
> This revision is now accepted and ready to land.
>
> LGTM after change in inline comment.
>
>
>
> Comment at: include/__hash_table:114
> @@ +113,3 @@
> +template
>
EricWF accepted this revision.
EricWF added a comment.
This revision is now accepted and ready to land.
LGTM after change in inline comment.
Comment at: include/__hash_table:114
@@ +113,3 @@
+template
+struct __can_extract_key<_Pair, _Key, pair<_First, _Second>>
+: conditio
dexonsmith updated this revision to Diff 50883.
dexonsmith added a comment.
Eric sent me his preferred tests, which look fine to me. I've applied them to
this patch.
http://reviews.llvm.org/D16360
Files:
include/__hash_table
test/libcxx/containers/unord/unord.set/insert_dup_alloc.pass.cpp
On Wed, Mar 16, 2016 at 1:31 PM, Duncan P. N. Exon Smith <
dexonsm...@apple.com> wrote:
>
> > On 2016-Mar-16, at 12:20, Eric Fiselier wrote:
> >
> > EricWF added a comment.
> >
> > Adding inline comments for the implementation. Comments on the tests to
> follow shortly.
> >
> >
> > ==
Great! I think we should proceed with your is_trivially_copyable idea and
I'll see if I can come up with a pathological test case that breaks it.
Hopefully we can find something that works.
On Mar 17, 2016 2:54 PM, "Duncan P. N. Exon Smith via cfe-commits" <
cfe-commits@lists.llvm.org> wrote:
> r
> On 2016-Mar-16, at 12:20, Eric Fiselier wrote:
>
> EricWF added a comment.
>
> Adding inline comments for the implementation. Comments on the tests to
> follow shortly.
>
>
>
> Comment at: include/__hash_table:103
> @@ -102,1 +102,3 @@
>
> +template
> +struct __extract_k
dexonsmith updated this revision to Diff 50869.
dexonsmith added a comment.
Eric and I had a quick chat on IRC.
- The asymmetric usage of __extract_key and __can_extract_key was awkward, as
Eric pointed out already.
- However, a symmetric __extract_key would have caused Clang (and other
compile
EricWF added a comment.
No worries, I'll try and take a look this weekend. Thanks.
http://reviews.llvm.org/D16360
___
cfe-commits mailing list
cfe-commits@lists.llvm.org
http://lists.llvm.org/cgi-bin/mailman/listinfo/cfe-commits
dexonsmith updated this revision to Diff 50514.
dexonsmith added a comment.
A few updates:
- Rebased on top of http://reviews.llvm.org/D18115.
- Rewrote the test to match suggested style.
- Updated the code to catch unordered_set::emplace as well, using a
__can_extract_key trait and a companion
dexonsmith added a comment.
Honestly I didn't think much about the testing style. I used r239666
as a reference and modified it for std::unordered_map.
I'll redo the tests when I have a moment, based on the newer examples.
Probably next week some time?
http://reviews.llvm.org/D16360
___
Honestly I didn't think much about the testing style. I used r239666
as a reference and modified it for std::unordered_map.
I'll redo the tests when I have a moment, based on the newer examples.
Probably next week some time?
> On 2016-Feb-19, at 17:53, Eric Fiselier wrote:
>
> EricWF added a c
EricWF added a comment.
@dexonsmith Actually is it OK if I contribute the tests for this patch? Your's
are in no way bad, but I want to test this in a similar manner to
`unord.map.modifiers/insert_allocator_requirments.pass.cpp` and it's not fair
to ask you to do that.
However if your willing
EricWF added a comment.
Overall this patch is *almost* there. My only objection is that this
optimization neglects "unordered_set". Optimally we would also catch
"unordered_set.emplace(42)"
Comment at: include/__hash_table:103
@@ -102,1 +102,3 @@
+template ::type>
-
dexonsmith updated this revision to Diff 47936.
dexonsmith added a comment.
Updated the patch to apply against r260513. The logic is much simpler now;
thanks Eric for the cleanup.
Let me know if there's anything else to do!
http://reviews.llvm.org/D16360
Files:
include/__hash_table
test/
dexonsmith added a comment.
Marshall, thanks for the link to #2464. That does look scary.
Nevertheless, I think this -- and the other over-eager allocations
in {unordered_,}{multi,}{map,set} -- are worth optimizing as long
as we can do it in a standards-compliant way. It's a pretty serious
regr
Marshall, thanks for the link to #2464. That does look scary.
Nevertheless, I think this -- and the other over-eager allocations
in {unordered_,}{multi,}{map,set} -- are worth optimizing as long
as we can do it in a standards-compliant way. It's a pretty serious
regression in performance compare
dexonsmith updated this revision to Diff 46603.
dexonsmith added a comment.
Eric, I think this addresses all of your review comments.
There are a couple of prep NFC commits that I sent for review:
http://lists.llvm.org/pipermail/cfe-commits/Week-of-Mon-20160201/148661.html
http://lists.llvm.org/p
mclow.lists added a comment.
I don't have any comments on this code at this time, but I want to caution
people that this part of the standard library is **extremely** carefully
specified, and meeting all the requirements is a fiddly bit of work.
For an example of this, look at LWG issue #2464,
EricWF added a comment.
In http://reviews.llvm.org/D16360#334781, @dexonsmith wrote:
> Great, I should have time to clean this up tomorrow afternoon.
>
> Regarding emplace, this patch as is has tests for emplace, but
> they depend on r258575 being in tree. You asked me to revert
> that... I'll
On Sun, Jan 24, 2016 at 9:46 PM, Duncan P. N. Exon Smith <
dexonsm...@apple.com> wrote:
> Great, I should have time to clean this up tomorrow afternoon.
>
> Regarding emplace, this patch as is has tests for emplace, but
> they depend on r258575 being in tree. You asked me to revert
> that... I'll
dexonsmith added a subscriber: dexonsmith.
dexonsmith added a comment.
Great, I should have time to clean this up tomorrow afternoon.
Regarding emplace, this patch as is has tests for emplace, but
they depend on r258575 being in tree. You asked me to revert
that... I'll wait for your response ov
Great, I should have time to clean this up tomorrow afternoon.
Regarding emplace, this patch as is has tests for emplace, but
they depend on r258575 being in tree. You asked me to revert
that... I'll wait for your response over in that thread:
http://lists.llvm.org/pipermail/cfe-commits/Week-of-M
EricWF added a comment.
Alright, so this is looking really good. Once we land this would you be
interested in applying the same optimization to `emplace` calls?
Comment at: include/__hash_table:863
@@ -862,2 +862,3 @@
_LIBCPP_INLINE_VISIBILITY
-pair __insert_unique_va
dexonsmith updated this revision to Diff 45758.
dexonsmith added a comment.
Committed r258511 and r258575 as preps. Updated patch addresses review
comments (and skips the trivially constructible parts).
http://reviews.llvm.org/D16360
Files:
include/__hash_table
include/unordered_map
tes
I'll upload a new patch in a moment. Replies inline below.
> On 2016-Jan-21, at 23:12, Eric Fiselier wrote:
>
> EricWF added a comment.
>
> Overall the patch looks good but I have a few concerns.
>
>> - If argument.first can be trivially converted to key_type, don't alloc.
>
>
> I'm concern
> On 2016-Jan-21, at 23:14, Eric Fiselier wrote:
>
> EricWF added a comment.
>
>> - Did I successfully match the coding style? (I'm kind of lost without
>> clang-format TBH.)
>
>
> The style looks pretty good. I'll comment on any nits I have.
>
>> - Should I separate the change to __constru
EricWF added a comment.
> - Did I successfully match the coding style? (I'm kind of lost without
> clang-format TBH.)
The style looks pretty good. I'll comment on any nits I have.
> - Should I separate the change to __construct_node_hash() into a separate
> prep commit? (I would if this were
EricWF added a reviewer: mclow.lists.
EricWF added a subscriber: mclow.lists.
EricWF added a comment.
Adding @mclow.lists as a reviewer.
http://reviews.llvm.org/D16360
___
cfe-commits mailing list
cfe-commits@lists.llvm.org
http://lists.llvm.org/cgi
EricWF added a comment.
Overall the patch looks good but I have a few concerns.
> - If argument.first can be trivially converted to key_type, don't alloc.
I'm concerned with this part of the change because:
- The `is_trivially_*` traits are often not available and can sometimes blow up.
- It a
dexonsmith created this revision.
dexonsmith added a reviewer: EricWF.
dexonsmith added a subscriber: cfe-commits.
(Repost of:
http://lists.llvm.org/pipermail/cfe-commits/Week-of-Mon-20160118/147379.html
on Phabricator as requested by Eric.)
This is a follow-up to r239666: "Fix PR12999 - unordere
34 matches
Mail list logo
