- User Since
- Jul 16 2012, 3:06 PM (392 w, 2 h)
Sat, Jan 4
Thu, Jan 2
What is the benefit to the user to have this constructor SFINAE away?
With static_assert we can craft an error message.
Wed, Jan 1
I'm fine with the other changes here, but not the array change.
Sun, Dec 29
This looks good to me now.
You need to update www/cxx17_status.html appropriately as well.
Thu, Dec 26
Dec 17 2019
Dec 16 2019
There's at least three things going on in this patch; and that makes it harder to review:
- Rename all the index_types to size_type.
- Add static_assert to several cases
- Guard against overflow in span::subspan (ostensibly the purpose of this patch).
Huh. I thought I fixed that typo. Anyway this LGTM.
Dec 12 2019
Is this related to https://bugs.llvm.org/show_bug.cgi?id=44145 ?
Dec 1 2019
I don't think that https://bugs.llvm.org/show_bug.cgi?id=43013 is actually a bug.
Nov 27 2019
This is a mess.
The changes in libcxx/CMakeLists.txt are unrelated to P0616.
The changes in algorithm are also unrelated to P0616.
The changes in forward_list are also unrelated to P0616, and are based on an outdated version of that file.
You proposed the changes in numeric before (in D69286) and then never came back to them.
And you have no tests.
Nov 25 2019
Nov 18 2019
Nov 15 2019
The code here looks fine to me, but I want to think about the tests for a bit.
Thanks for the patch.
Nov 14 2019
One feature per patch, please.
Sorry I'm late to the party; I've been traveling for 3+ weeks.
I would like to be reassured that the following code will not warn:
I did some playing with compiler explorer, and got even better codegen by sprinkling casts to _Up over the calculation to __half_diff.
This only makes a difference if _Tp is smaller than int.
Nov 11 2019
Nov 2 2019
This looks reasonable to me, but I noticed that there's a behavior difference - __libcpp_locale_guard has been added. (previously the loc parameter was ignored).
Nov 1 2019
Oct 31 2019
The tests should be more comprehensive; they should ASSERT_SAME_TYPE(decltype(s), XXX) to make sure that the deduction guides actually return the correct type, rather than just compile.
Oct 30 2019
Oct 27 2019
This looks good to me.
Shorter code, still no branches; passes all the tests.
Oct 26 2019
Oct 22 2019
Oct 21 2019
Oct 20 2019
This looks fine to me, except that there's no test to show that it works.
Oct 11 2019
I'm fine with incremental changes.
It would be nice to have a test case for a allocator that throws on move-assignment, though.
Thinking about this some more, I agree with your analysis about the move assignment throwing.
Oct 10 2019
Nice use of __libcpp_is_constant_evaluated(), but how will that play for C++03?
This looks ok to me. If you're concerned about performance, you might want to split this into two routines, one for the case were the allocators are equal, and one for the case where they're not (and skip the whole allocator dance for the equal case.)
Oct 7 2019
I'm wondering where you are seeing getting this codegen from.
A couple of things:
- Can you give your patches a better title? "Optimize copy constructor" is not very informative. Which copy constructor?
- What does this do it the dylib - where basic_string<char> and basic_string<wchar_t> are externally instantiated?
Oct 3 2019
Thanks for doing this, Richard.
A few things:
- Needs tests (as you said)
- The config macro stuff is all wrong (we build that file from a template)
- Use __libcpp_is_constant_evaluated instead of __builtin_is_constant_evaluated because it doesn't have to be #ifdefed. (see r364873 and r364884)
- If we're in a consteval block, we shouldn't be testing for exceptions being disabled, right?
If the Android and FreeBSD folks are ok with this, I'm fine with it
Sep 26 2019
This is unfortunate and unnecessary. In particular, this breaks two clang tests if clang is configured to use libcxx as the default C++ standard library.
Sep 25 2019
Committed as revision 372896
Sep 24 2019
this looks fine to me now (with a nit).
Sep 23 2019
Sep 22 2019
Sep 13 2019
Sep 12 2019
This has been landed, and the breakage has been fixed.
This looks fine to me; please update www/cxx2a_status.html when you commit.
Sep 11 2019
It would be nice to have a test (in test/libcxx) to check that these annotations are in the expected places.
With the comment change I suggested, I am fine with this.
zoecarver accepted this revision
@zoecarver is not an approver for libc++.
I'm not a big fan of duplicated code; but especially in this case, since in D62453 you edit the duplicated code to add a throw; but you have to do it twice - once in __parse_backref and once in the new code. Why is that preferable?
Sep 10 2019
Maybe I'm missing something, but I don't see any synchronization between two syncstreams wrapping the same stream.
A general comment about the tests: You've got a bunch of methods marked "noexcept", but nowhere to I see ASSERT_NOEXCEPT or ASSERT_NOT_NOEXCEPT in the tests to check that.
Sep 7 2019
LGTM with the one change I suggested.
I'm wondering if this has become complicated enough that we should define a _LIBCPP_C_HAS_NO_GETS config macro.
Sep 5 2019
Sep 4 2019
I see maintaining an explicit list of exports as an ongoing time sink - are you sure this is the the direction that we want to take?
Sep 3 2019
If you're going to do __add__lvalue_reference, __add_rvalue_reference, and __remove_reference, why not go all the way and add __is_reference, __is_lvalue_reference and __is_rvalue_reference?
Aug 26 2019
Aug 23 2019
The templates basic_streambuf<char> and basic_streambuf<wchar> are externally instantiated in the libc++.dylib.
Marking them as inline would remove them from the dylib, breaking any existing binaries that refer to them.
Aug 21 2019
This seems to work for me (pthreads and nothreads).
In general, I think we're playing "whack-a-mole' with this chunk of code, and I've become fairly unhappy with it.
Aug 20 2019
Committed as revision 369399.
This is an alternative to D66301
I put an alternate solution up as D66480
Aug 19 2019
Yes. Because then it would be in the global namespace.