Bug 174921

Created attachment 318003 [details] the patch

Created attachment 318004 [details] the patch

Attachment 318004 [details] did not pass style-queue: ERROR: Source/WTF/wtf/CagedUniquePtr.h:84: Should be indented on a separate line, with the colon or comma first on that line. [whitespace/indent] [4] ERROR: Source/WTF/wtf/CagedUniquePtr.h:135: Should be indented on a separate line, with the colon or comma first on that line. [whitespace/indent] [4] Total errors found: 2 in 13 files If any of these errors are false positives, please file a bug against check-webkit-style.

Created attachment 318012 [details] the patch

Attachment 318012 [details] did not pass style-queue: ERROR: Source/WTF/wtf/CagedUniquePtr.h:84: Should be indented on a separate line, with the colon or comma first on that line. [whitespace/indent] [4] ERROR: Source/WTF/wtf/CagedUniquePtr.h:135: Should be indented on a separate line, with the colon or comma first on that line. [whitespace/indent] [4] Total errors found: 2 in 13 files If any of these errors are false positives, please file a bug against check-webkit-style.

Created attachment 318016 [details] the patch Fix windows.

Attachment 318016 [details] did not pass style-queue: ERROR: Source/WTF/wtf/CagedUniquePtr.h:84: Should be indented on a separate line, with the colon or comma first on that line. [whitespace/indent] [4] ERROR: Source/WTF/wtf/CagedUniquePtr.h:135: Should be indented on a separate line, with the colon or comma first on that line. [whitespace/indent] [4] Total errors found: 2 in 13 files If any of these errors are false positives, please file a bug against check-webkit-style.

Created attachment 318043 [details] the patch

Attachment 318043 [details] did not pass style-queue: ERROR: Source/WTF/wtf/CagedUniquePtr.h:84: Should be indented on a separate line, with the colon or comma first on that line. [whitespace/indent] [4] ERROR: Source/WTF/wtf/CagedUniquePtr.h:135: Should be indented on a separate line, with the colon or comma first on that line. [whitespace/indent] [4] Total errors found: 2 in 13 files If any of these errors are false positives, please file a bug against check-webkit-style.

Comment on attachment 318043 [details] the patch r=me

Comment on attachment 318043 [details] the patch iew in context: https://bugs.webkit.org/attachment.cgi?id=318043&action=review r=me > Source/WTF/wtf/CagedUniquePtr.h:82 > +class CagedUniquePtr<kind, T[], typename std::enable_if<std::is_trivially_destructible<T>::value>::type> : public CagedPtr<kind, T> { It's neat that you don't need the size here! :D > Source/WTF/wtf/CagedUniquePtr.h:104 > + new (result + count) T(std::forward<Arguments>(arguments)...); I think you want to construct in increasing order. That's usually what C++ containers do, and it's observable by the values being constructed. > Source/WTF/wtf/CagedUniquePtr.h:133 > +class CagedUniquePtr<kind, T[], typename std::enable_if<!std::is_trivially_destructible<T>::value>::type> : public CagedPtr<kind, T> { This inherits CagedPtr's operator== but does the wrong with by not comparing m_count. That should never be an issue because you shouldn't be able to have to unique pointers to the same base, but different sizes. Right? > Source/WTF/wtf/CagedUniquePtr.h:157 > + new (result + count) T(std::forward<Arguments>(arguments)...); Ditto, construct in increasing order. > Source/WTF/wtf/CagedUniquePtr.h:183 > + this->m_ptr[m_count].~T(); This reverse order destruction is good though. > Source/WTF/wtf/Gigacage.cpp:127 > + if (checkedSize.hasOverflowed()) UNLIKELY

(In reply to JF Bastien from comment #11) > Comment on attachment 318043 [details] > the patch > > iew in context: > https://bugs.webkit.org/attachment.cgi?id=318043&action=review > > r=me > > > Source/WTF/wtf/CagedUniquePtr.h:82 > > +class CagedUniquePtr<kind, T[], typename std::enable_if<std::is_trivially_destructible<T>::value>::type> : public CagedPtr<kind, T> { > > It's neat that you don't need the size here! :D > > > Source/WTF/wtf/CagedUniquePtr.h:104 > > + new (result + count) T(std::forward<Arguments>(arguments)...); > > I think you want to construct in increasing order. That's usually what C++ > containers do, and it's observable by the values being constructed. I'll leave it for now. I already tested it this way, and I don't think that any of our code cares. > > > Source/WTF/wtf/CagedUniquePtr.h:133 > > +class CagedUniquePtr<kind, T[], typename std::enable_if<!std::is_trivially_destructible<T>::value>::type> : public CagedPtr<kind, T> { > > This inherits CagedPtr's operator== but does the wrong with by not comparing > m_count. That should never be an issue because you shouldn't be able to have > to unique pointers to the same base, but different sizes. Right? Right! I think it's "correct" but I put in a FIXME and cited https://bugs.webkit.org/show_bug.cgi?id=175541 > > > Source/WTF/wtf/CagedUniquePtr.h:157 > > + new (result + count) T(std::forward<Arguments>(arguments)...); > > Ditto, construct in increasing order. > > > Source/WTF/wtf/CagedUniquePtr.h:183 > > + this->m_ptr[m_count].~T(); > > This reverse order destruction is good though. > > > Source/WTF/wtf/Gigacage.cpp:127 > > + if (checkedSize.hasOverflowed()) > > UNLIKELY

Landed in https://trac.webkit.org/changeset/220712/webkit

<rdar://problem/33879627>