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