- User Since
- Feb 11 2015, 3:26 PM (200 w, 4 h)
Mon, Dec 10
Could you please split this change and the changes from class to struct into different patches? That would make it easier to review.
LGTM with the fixes to the tests.
Oh gosh I've been wondering forever why we defined them as classes in the standard. Glad to see we're fixing that.
Fri, Dec 7
I think I'm fine with this, but I'd still like to better understand the positive performance impact of the change. When we spoke @EricWF said he could provide an interpretation.
If you use some of my suggestions, please make sure it compiles in C++03 mode. I might be using some C++11 features (not sure whether default argument for template parameters is C++03).
I believe this patch fixes an important QOI bug: see http://llvm.org/PR37574. I wholeheartedly agree with Eric that allocators-over-const are an abomination, however pointers-to-const are a fine thing and our implementation should handle them gracefully.
- Just to make sure I understand; this patch has nothing to do with https://reviews.llvm.org/D48342, they are entirely orthogonal. Is this correct?
- Also, before this patch, the allocator's construct and destroy were NEVER called in C++03, but were called when available in C++11. After this patch, they are called when available in C++03 and in C++11. Is this correct?
So... the way we planned on tackling the hidden visibility problem is https://reviews.llvm.org/D54810 (which is blocked on a couple of Clang bugs w.r.t. visibility attributes). Would https://reviews.llvm.org/D54810 solve your problem?
Thu, Dec 6
Split libc++ specific tests into test/libcxx
I'll push this as I'm fairly sure this won't affect the (already broken) build on GCC. At least not as far as I can tell.
Committed as r348509.
Ahh, you're right. Ok, LGTM.
Committed as r348485.
Wed, Dec 5
Two overarching comments.
Tue, Dec 4
I'm going to closely monitor potential CI failures. I think only CI on Apple platforms should be affected, but I'll keep an eye out.
@dyaroshev Can you please rebase on top of master? There's been significant changes in algorithms.bench.cpp and the patch does not apply cleanly anymore.
I'll apply the patch.
I think we need to use std::launder when accessing the function object through the small buffer. This is a problem we seem to have in the current implementation too.
Mon, Dec 3
Eric and I just spoke. My point of view:
Fri, Nov 30
Thu, Nov 29
@mclow.lists This should fix the test failures you've been seeing. Would you please kindly test the patch on your system? I've tested the patch under several configurations but I'd like to make sure.
I'd like to merge this early in the cycle to give vendors the chance to notice failures in case there are any (but we don't expect that to happen given our investigation). I'll merge this now because I'm not aware of any other vendor that could be worried by this (and so the waiting time implied by Duncan's question above is potentially unbounded). However, if anyone has concerns, please comment here and I'll insta-revert until we've come to an agreement.
Added release notes.
I'd like to see benchmarking results that show the benefit of this approach.