[patch] [libcxx] possible version mistake in deque - sent by mistake. ignore will revise.

[G M]

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 <arthur.j.odwyer at gmail.com>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 <gmisocpp at gmail.com> wrote:
> >
> > _______________________________________________
> > cfe-commits mailing list
> > cfe-commits at cs.uiuc.edu
> > http://lists.cs.uiuc.edu/mailman/listinfo/cfe-commits
> >
>
-------------- next part --------------
An HTML attachment was scrubbed...
URL: <http://lists.llvm.org/pipermail/cfe-commits/attachments/20131009/f18dd60b/attachment.html>