Sorry Everybody I'm horrified to see I accidentally sent the wrong diff.
None of that was ready for submission and definitely shouldn't have all been in the same patch. I just meant to send changes for dequeue Thanks Arthur, I will look at your comments and send another patch related to that. On Wed, Oct 9, 2013 at 8:07 AM, Arthur O'Dwyer <[email protected]>wrote: > My inline comments: > > > (1) > > _LIBCPP_NEW_DELETE_VIS void* operator new(std::size_t __sz) > >-#if !__has_feature(cxx_noexcept) > >+#if defined(_LIBCPP_MSVC) > >+ throw(...) > >+#elif !__has_feature(cxx_noexcept) > > throw(std::bad_alloc) > > #endif > > ; > > Shouldn't the above diff be more like > > > _LIBCPP_NEW_DELETE_VIS void* operator new(std::size_t __sz) > > #if !__has_feature(cxx_noexcept) > >+#if defined(_LIBCPP_MSVC) > >+ throw(...) > >+#else > > throw(std::bad_alloc) > >+#endif > > #endif > > ; > > ? > > (2) > >- int value(char_type __ch, int __radix) const > >- {return __value(__ch, __radix);} > >+ int set_value(char_type __ch, int __radix) const > >+ {return __set_value(__ch, __radix);} > > This looks like an actively bad change. The Standard requires the name > of this member to be "value", not "set_value". What am I missing? > http://en.cppreference.com/w/cpp/regex/regex_traits/value > > (3) > >-#include <Windows.h> > >+//#include <Windows.h> > > Just remove the line, if it's really unnecessary; but this looks like > a change you were tentatively testing, and might not be intentional. > > (4) > >+ const uint32_t low = mask & 0x00000000FFFFFFFF; // Low order 32 bits. > > Maybe there's some library-implementation magic going on here, but I > don't see why you don't just remove everything after the '&', > inclusive. Question for the language gurus: isn't 0x00000000FFFFFFFF > the same as 0xFFFFFFFF in both type and value, in all supported > language dialects? > > (5) > >- __thread_id() _NOEXCEPT : __id_(0) {} > >+ __thread_id() _NOEXCEPT : __id_() {} > > >- thread() _NOEXCEPT : __t_(0) {} > >+ thread() _NOEXCEPT : __t_() {} > > >- thread(thread&& __t) _NOEXCEPT : __t_(__t.__t_) {__t.__t_ = 0;} > >+ thread(thread&& __t) _NOEXCEPT : __t_(__t.__t_) {__t.__t_ = > pthread_t();} > > We want zero-initialization here, not default-initialization. Are all > of these changes actually performing zero-initialization? Are they > necessary? > > (6) > >- bool joinable() const _NOEXCEPT {return __t_ != 0;} > >+ bool joinable() const _NOEXCEPT {return memcmp(&__t_, 0, > sizeof(__t_)) != 0;} > > This is just flatly a bug, and I'm shocked that the compiler didn't > diagnose it. > > (7) > > template<typename _Tp> char &__is_polymorphic_impl( > >- typename enable_if<sizeof((_Tp*)dynamic_cast<const volatile > void*>(declval<_Tp*>())) != 0, > >+ typename enable_if< > >+#if defined(_MSC_VER) && ! defined(__clang__) > >+ __is_polymorphic(_Tp*) > >+#else > >+ sizeof((_Tp*)dynamic_cast<const volatile void*>(declval<_Tp*>())) != > 0 > >+#endif > >+ , > > Is this working around a bug in MSVC's treatment of dynamic_cast<>, or > what's going on here? If the old code's enable_if expression didn't > work on MSVC, then this is a pretty big bug that deserves an > explanatory comment at the point where the workaround is introduced. > > (8) > >- int32_t __id_; > >+ long __id_; > > Is this whole chunk of code under #ifdef(_WIN32)? because your patch > changes the size of this structure on targets where long is 64-bit. > > > my $.02, > –Arthur > > > On Tue, Oct 8, 2013 at 8:02 AM, G M <[email protected]> wrote: > > > > _______________________________________________ > > cfe-commits mailing list > > [email protected] > > http://lists.cs.uiuc.edu/mailman/listinfo/cfe-commits > > >
_______________________________________________ cfe-commits mailing list [email protected] http://lists.cs.uiuc.edu/mailman/listinfo/cfe-commits
