- User Since
- Jul 16 2012, 3:06 PM (421 w, 1 d)
Thu, Aug 6
A couple of quick comments, more to come.
Thu, Jul 30
Tue, Jul 21
I see no tests.
Sat, Jul 18
Jun 30 2020
Jun 29 2020
General comment: would it not be better to make the compiler smarter to solve this (and reap the benefits for vector and other code)?
Jun 16 2020
I wouldn't change the __advance definitions. That's just spreading the pain around. Convert from Distance to iterator_traits<...>::difference_type in advance
Jun 15 2020
Jun 4 2020
So I don't understand what the observable difference is here. Could you add a test?
Jun 3 2020
I've got nothing to say here.
May 21 2020
LGTM. Feel free to ignore the clang-format whining.
This is not sufficient; it just "moves the pain" from the callers code into the dylib. All the functions in charconv.cpp need to be marked as noexcept.
May 6 2020
BTW, string(nullptr, 0) is a valid call. The range [nullptr, nullptr) is valid.
May 5 2020
You're not really solving the problem you're talking about.
Feb 24 2020
This looks fine to me.
Feb 16 2020
Feb 5 2020
Jan 23 2020
Still no tests. This will not be committed w/o tests.
Jan 20 2020
You have no tests.
If you had tests, you would probably have noticed that you typed _UP when you meant _Up
Jan 4 2020
Jan 2 2020
What is the benefit to the user to have this constructor SFINAE away?
With static_assert we can craft an error message.
Jan 1 2020
I'm fine with the other changes here, but not the array change.
Dec 29 2019
This looks good to me now.
You need to update www/cxx17_status.html appropriately as well.
Dec 26 2019
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.