[patch] [libcxx] possible version mistake in deque

[Arthur O'Dwyer]

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 <gmisocpp at gmail.com> wrote:
>
> _______________________________________________
> cfe-commits mailing list
> cfe-commits at cs.uiuc.edu
> http://lists.cs.uiuc.edu/mailman/listinfo/cfe-commits
>