+cfe-commits (This looks like a broken feature of Phab. I suggest not using it, since it hasn't put the patch context in this email, it didn't CC the list, and it spawned a new thread instead of replying to the commit: http://lists.llvm.org/pipermail/cfe-commits/Week-of-Mon-20160118/147763.html
I'll reply to that mail in a moment to link to this thread.) > On 2016-Jan-22, at 20:13, Eric Fiselier <e...@efcs.ca> wrote: > > EricWF added a subscriber: EricWF. > EricWF raised a concern with this commit. > EricWF added a comment. > > @dexonsmith This change should have been reviewed before committing. Sorry about this. I had assumed LLVM's culture of post-commit review. I'll take things more slowly going forward. > I think there are better ways to make `insert` and `emplace` work together. > In particular I would like to put most of the logic in `<__hash_table>` not > `<unordered_map>`. Given that __hash_table and unordered_map have incompatible concepts of "value_type", and the purpose of sharing them is to optimize away mallocs when we can pull out the right parts of value_type, I'm skeptical. > Please revert this commit. r258625 > /libcxx/trunk/include/unordered_map:930 I think we can just call > `__table_.__insert_unique(std::forward<_Arg>(__arg))` for the one arg case > and `__table_. The purpose of this commit was to ensure that emplace gets the same malloc optimizations as insert, so that the tests in this patch pass: http://lists.llvm.org/pipermail/cfe-commits/Week-of-Mon-20160118/147766.html I don't see how this alternate approach would accomplish that? > /libcxx/trunk/include/unordered_map:932 I think this optimization should be > limited to when "_Arg" is "value_type". The patch linked above optimizes mallocs whenever key_type matches. I also have a follow-up (originally part of that patch) that strengthens the optimization to whenever we can convert to key_type (and destruct it) trivially. It's simplest to forward to insert() to guarantee that each behaves "as if" the other were called; otherwise that patch needs to duplicate all the type-based dispatch in two places. > * `is_constructible` can cause a hard compile error when using Clang. I > prefer not to use it when possible. That's terrible. In what versions? (If ToT, we should fix that... can you point me at a PR?) It's already used in the enable_if for `insert()`. Does it break there? (My experience is that hard compile errors usually prevent meta-functions from being used in enable_ifs; I'd assumed there weren't any bugs in is_constructible since I saw the use below.) Maybe I should pivot the malloc optimization; if we change `insert()` to just call `emplace()` -- valid because of the enable_if (I assume the bugs in is_constructible don't cause miscompiles?) -- I can do the malloc optimization inside the single-argument `emplace()`. This would require more intrusive changes to __hash_table of course, since I think your original commit (r239666) only optimized the insert-side. What do you think? > > > Users: > dexonsmith (Author) > EricWF (Auditor) > > http://reviews.llvm.org/rL258575 > > > _______________________________________________ cfe-commits mailing list cfe-commits@lists.llvm.org http://lists.llvm.org/cgi-bin/mailman/listinfo/cfe-commits
