- User Since
- Feb 11 2015, 3:26 PM (178 w, 6 d)
Split the definition of _LIBCPP_ALWAYS_INLINE into compiler-specific sections
of __config, per Marshall's comment.
Mon, Jul 16
Fri, Jul 13
Made sure the attributes worked properly on GCC.
LGTM -- great! Just make sure the commit message does not pretend the change includes merge.
Thu, Jul 12
Wed, Jul 11
Pushing since I got an offline O.K. from Eric.
I forgot to mention it in my initial review, but it also seems like you forgot to update all the synopsis in the comments at the beginning of headers.
Generally looks pretty good, but I have a couple questions/suggestions.
Tue, Jul 10
Mon, Jul 9
I've now managed to run the check-cxx-abilist test on my machine and it passes. I'd like to commit this again, @EricWF am I good to go?
Regarding whether we want to enable or disable those warnings by default: it seems like we've reached consensus (on the LLVM IRC) for enabling the deprecation warnings by default. However, I suggest we keep them disabled by default in this commit, and then change the default in a subsequent change. This will make things easier from an integration perspective, and also in case we need to roll back the defaults because of some unforeseen circumstances.
Fri, Jul 6
This revision to the patch fixes a problem where __pbump had been applied
_LIBCPP_EXTERN_TEMPLATE_INLINE_VISIBILITY instead of _LIBCPP_INLINE_VISIBILITY,
which caused the symbols exported in the ABI to change and broke the CI.
Thu, Jul 5
I'm dropping this because I reverted https://reviews.llvm.org/D48892 until I figure out why it's breaking LLDB. The next time I submit https://reviews.llvm.org/D48892, I'll make sure to do it without this problem.
I reverted this commit. Sorry for the blunder. I'll take a look at why LLDB's tests are doing this.
It's pretty obvious that this is the problem when looking at https://reviews.llvm.org/D48892 (grep for __pbump), but TBH I haven't been able to run the check-cxx-abilist test locally. Mine fails with like 500 differences, so there's got to be something I'm doing wrong.
Wed, Jul 4
Before the patch, the backtrace refers to the instantiation of the set's destructor instead of referring to the set's instantiation itself:
FWIW, I would prefer to enable deprecation warnings by default and to have a way of turning them off -- so as to be as close to the standard as possible by default. But I'm fine either way, and this is always a choice we can revisit later by just switching the default from opt-in to opt-out.
Addressed reviewer feedback.
Tue, Jul 3
Note that this revision also removes _LIBCPP_ALWAYS_INLINE because it is not used anymore and I want to avoid any potential confusion. If we do come around a need to mark something as always inline in the future, we should reintroduce the macro for that use case. I'm not sure such a use case will come any time soon, though.
Note that even in case we end up renaming _LIBCPP_INLINE_VISIBILITY to something else, this patch is useful because it ensures that nothing was _actually_ meant to be always inline. So any potential rename can then be just that -- a mechanical rename with no potential for semantic change.
Fri, Jun 29
Thu, Jun 28
I guess I'm echoing some of Eric's comments, but does it make sense to build libc++abi against a system-installed libc++? If not, this change makes sense.
Wed, Jun 27
More missed noexcepts.
LGTM. All comments/questions are just for my education. Other things I did: double-check that you changed all the functions changed by https://cplusplus.github.io/LWG/lwg-defects.html#2946.
I can't commit this myself because I don't have commit rights yet.
Tue, Jun 26
May 26 2016
Rebase the patch on top of the current master. The patch passes all of Clang's tests (make check-clang). I also removed the TODO comment about diagnostics, since that was answered by Richard Smith.
May 25 2016
No problem for the delay; we're all busy :-). Unfortunately, the patch does not apply cleanly on master anymore, so I'll rebase it and someone can then commit it for me.
May 10 2016
Feb 15 2016
Address some remaining issues:
- Rename nth_element to type_pack_element, as suggested by Richard Smith
- Fix template parameter position issue in createTypePackElementParameterList
Jan 14 2016
Address Richard Smith's review comments:
- Remove IndexType parameter, and make Index a std::size_t
- Remove assertion about APSInt::GetExtValue()
- Other style changes
Jan 13 2016
Rebase on top of the master branch, and add Richard Smith as a reviewer, since he also reviewed David Majnemer's original patch (suggested by Nathan Wilson).
Dec 21 2015
This LGTM. I also just confirmed that it fixes the original issue in Hana that caused me to report PR22806.
Dec 10 2015
We should probably consider changing the name from __nth_element to something else, perhaps something that contains the word "pack". This is especially true if we are to implement other intrinsics for manipulating parameter packs, like slicing (as proposed by Eric Fiselier). Unless someone with a clearer view of the project suggests something, I would probably go with __pack_element (and then we could add __pack_slice, etc...). I have no strong preference.
Added full lines of context. Sorry for not doing it the first time around.
Nov 26 2015
Mar 22 2015
As a motivation: if we get through this review and we ever meet at a conference, I'm buying both of you guys a beer. Let's finish this!
Mar 20 2015
Feb 12 2015
Feb 11 2015
The faulty code was mine. With the pair example, it is clear that all those is_default_constructible<T> are instantiated even when the default constructor is not. I think my intention when writing Dummy && is_default_constructible<_Tp>::value was to delay the instantiation of is_default_constructible<_Tp> as a compile-time optimization (which it fails to do), but not for correctness purposes.