- User Since
- Jul 16 2012, 3:06 PM (344 w, 1 d)
Tue, Feb 12
I think this is good to go!
Mon, Feb 11
This looks fine to me - thanks!
This is all pretty much a lost cause on Windows anyway, since none of the compiler intrinsics are constexpr, and with the (upcoming) adoption of P0553 and P0556 these routines will have to be constexpr.
Sun, Feb 10
I haven't looked at any of the "nonsense" yet.
Thu, Feb 7
Wed, Feb 6
Almost there - down to nits
Tue, Feb 5
The title of https://wg21.link/LWG2735 is "std::abs(short), std::abs(signed char) and others should return int instead of double in order to be compatible with C++98 and C"
No one is "forced" to call __sift_down. In fact, it's explicitly forbidden by the standard.
I think all of these are no-brainers except the one calling __throw_failure.
Arthur - I'm sorry to say that this will go nowhere.
You're introducing non-reserved identifiers into libc++, which can cause legal code to break.
Mon, Feb 4
Oh, I agree. However, when I have proposed this in the past, I have gotten pushback from many people (including Apple, Google and others).
At the very least, you should announce this on llvm-dev, and get feedback there.
@jfb, I am not going to look at all of these changes. If you assure me that they all look like this: https://reviews.llvm.org/differential/changeset/?ref=1324002
and that the test suite still passes on the desktops, I'm fine with you applying this.
My take on this is
Fri, Feb 1
@rsmith just reminded me (in IRC) that it's perfectly legal to declare operator new in your classes.
My problem with declaring our own placement new is that I think we're just kicking the can down the road; that the problem might (will) come back to bite us in the future.
It would certainly be conforming for clang to reject this code at compile time.
This looks fine to me. It also looks (to me) that it will make applying https://wg21.link/P1328 cleaner (assuming it gets approved)
Update based on Eric's comments.
Thu, Jan 31
Committed as revision 352781.
Wed, Jan 30
Turns out the placement versions of new are not replaceable, so what I said about UB is just wrong.
Other than the missing // UNSUPPORTED bits, this looks good to me.
Do you need me to commit this?
Um... I suspect that this is technically UB. Having more than two (one in the standard library, and one other) in an executable context is a bad idea.
Marking it as inline might make it work, but I still think it's UB.
Tue, Jan 29
I just tried this (on Compiler Explorer) using LLVM 7, and the code for my original test in https://bugs.llvm.org/show_bug.cgi?id=35637 is now optimal.
Looking briefly at your test case, it seems to be fixed now too. Can you confirm or disprove, please?
Mon, Jan 28
This change breaks building on Mac OS X - at least for libc++.
The user visible failure is:
Thu, Jan 24
(Finally) committed this as revision 352087.
I cut out most of the random_shuffle_rand.pass.cpp test, because it relied on C++11 features, and didn't work for C++03.
If you want to re-submit the test as a separate patch, I promise to review it in a more timely manner.
Wed, Jan 23
For some reason, this test was not marked // XFAIL gnu-linux like all the other tests that use the LOCALE_cs_CZ_ISO8859_2 locale.
In revision 352006, I uncommented this entire test, and added the XFAIL line
Yes, this appears to have been landed. Closing.
I changed the _LIBCPP_TYPE_VIS_ONLY to _LIBCPP_TEMPLATE_VIS and committed this as revision 351993.
(Finally) committed as revision 351971
I have no problem with the code change, but no context on whether or not it is correct.
I'm hoping some other people familiar with ARM and DWARF can chime in.
Tue, Jan 22
Re-gen the diff after I updated the tests based on Louis' comments (see r351887)
Thanks for the quick review. I'm going to hold off on landing this until after Kona.
Mon, Jan 21
Jan 18 2019
Sigh. At least we've got a macro wrapping this; we only have to pore over this once.
Jan 15 2019
I am coming to like the idea that these tests should live in test/libcxx, not test/std, because they're all about libc++'s implementation status.
That being said, the tests are in test/std today, and this isn't an awful place for them.
This looks fine to me,
Jan 14 2019
I'm a bit concerned about the TEST_HAS_NO_SPACESHIP_OPERATOR and how it tracks with _LIBCPP_HAS_NO_SPACESHIP_OPERATOR, but I'm not going to hold this up for that.
this looks fine to me; but I'd like @EricWF to chime in here.
Jan 11 2019
Committed as revision 350972.
Committed as revision 350929
Jan 10 2019
Sorry this took me so long.
After removing the __VSTD::, I'm good with this.
Jan 9 2019
Added support for local_time in the calculations as well.
Jan 7 2019
Ok, some of those we define ourselves w/o checking.
What I'm saying is that this is a pattern we use throughout libc++, and I'd rather follow that pattern than explain why _LIBCPP_HAS_NO_ALIGNED_ALLOCATION is different from all these others.
This looks reasonable to me.
You should also update www/cxx2a_status.html to show that P0722 is "complete".
Another question - why do we need to check if clang supports destroying delete to define the type destroying_delete_t? Why not define it always, and just define the feature test macro when we have compiler support?
This is going to need some work before landing.
In general, we *do* protect against user-defined defines.
This, and defining stuff as negative (_LIBCPP_HAS_NO_XXX) lets users disable parts of the libc++ functionality by defining the macros.
If Saleem is happy with this, then I'm good too.
I like this much better now; making a note to myself to go back to <unordered_map> and <stdexcept> and use this there.
Oh, look, this is https://github.com/cplusplus/draft/issues/534, where JW wrote:
I'm fine with this as well - assuming the FreeBSD people are happy.
Dec 23 2018
Dec 18 2018
In other words, I fully agree that we should strive to run experimental tests as often as possible (when it makes sense), and we do have the right default here. But when I explicitly say "don't give me experimental tests", I don't want them to run.
Committed all of this except "poisoned_hash_helper.hpp" as revision 349566
Did you run these tests after adding the includes?
The "poisoned hash" tests fail - because poisoned_hash_helper.hpp now includes various hash specializations.
In fact, it causes clang to crash (I've reported PR 40093 for that).
I'll remove the changes to "poisoned_hash_helper.hpp" and see what happens.
I can't commit the patch myself: I don't have rights to do this.
The includes look fine. The static_casts (as always) look ugly. Once you make the changes that I've requested, you can commit this.
Dec 17 2018
So what do you suggest we do about this test?
I have written a similar program, but using t.translate_nocase. All the characters from C0 --> DE are translated on Darwin.
In my opinion we shall not test UB as each implementation can probably behave in any way.
On Mac OS, using the locale en_US.UTF-8, the call std::re_traits<char>().translate_nocase('\xDA') returns '\xFA'
This updated test fails on Mac OS X. (the assert on line 48 fires)
Amusingly enough, I received this bug report this morning, which appears be related.