Vector::releaseBuffer should return an OwnPtr
Created attachment 210538 [details] Patch
Attachment 210538 [details] did not pass style-queue: Failed to run "['Tools/Scripts/check-webkit-style', '--diff-files', u'Source/WTF/ChangeLog', u'Source/WTF/wtf/Vector.h', u'Source/WTF/wtf/text/StringBuffer.h', u'Source/WTF/wtf/text/StringImpl.h', u'Source/WebCore/ChangeLog', u'Source/WebCore/platform/network/cf/FormDataStreamCFNet.cpp']" exit_code: 1 Source/WTF/wtf/Vector.h:311: The return type should use PassOwnPtr instead of OwnPtr. [readability/pass_ptr] [5] Source/WTF/wtf/Vector.h:491: The return type should use PassOwnPtr instead of OwnPtr. [readability/pass_ptr] [5] Source/WTF/wtf/Vector.h:669: The return type should use PassOwnPtr instead of OwnPtr. [readability/pass_ptr] [5] Total errors found: 3 in 6 files If any of these errors are false positives, please file a bug against check-webkit-style.
Comment on attachment 210538 [details] Patch View in context: https://bugs.webkit.org/attachment.cgi?id=210538&action=review r=me > Source/WTF/ChangeLog:9 > + Change Vector::releaseBuffer() to return an OwnPtr. I intentionally chose > + to use an OwnPtr over a PassOwnPtr since we're trying to move away from PassOwnPtr objects. The future is now!
Committed r155121: <http://trac.webkit.org/changeset/155121>
FYI, It broke everything everywhere: - Apple Windows: http://build.webkit.org/builders/Apple%20Win%207%20Release%20%28Tests%29/builds/38030 - GTK: http://build.webkit.org/builders/GTK%20Linux%2064-bit%20Release/builds/40434 - Qt: http://build.webkit.org/builders/Qt%20Linux%20Release/builds/62669 - Maybe EFL too, but EFL bots haven't picked this revision up.
Comment on attachment 210538 [details] Patch View in context: https://bugs.webkit.org/attachment.cgi?id=210538&action=review > Source/WTF/wtf/Vector.h:316 > + return adoptPtr(buffer); This doesn’t seem write. If this was allocated with new, then adoptPtr makes sense, or if it was array new, then adoptArrayPtr makes sense. But I don’t think we have an adopt function that is designed to work with fastMalloc’d memory. > Source/WTF/wtf/Vector.h:1164 > + buffer = adoptPtr(static_cast<T*>(fastMalloc(bytes))); This doesn’t look safe. Can we allocate something with fastMalloc and then delete it with delete and expect it to work? > Source/WTF/wtf/text/StringBuffer.h:47 > + m_data = adoptPtr(static_cast<CharType*>(fastMalloc(m_length * sizeof(CharType)))); Ditto. > Source/WTF/wtf/text/StringBuffer.h:65 > + m_data = adoptPtr(static_cast<UChar*>(fastRealloc(m_data.release().leakPtr(), newLength * sizeof(UChar)))); Ditto. > Source/WTF/wtf/text/StringBuffer.h:82 > + OwnPtr<CharType> data = m_data.release(); > + return data.release(); Why the local variable?
Comment on attachment 210538 [details] Patch View in context: https://bugs.webkit.org/attachment.cgi?id=210538&action=review >> Source/WTF/wtf/Vector.h:316 >> + return adoptPtr(buffer); > > This doesn’t seem write. If this was allocated with new, then adoptPtr makes sense, or if it was array new, then adoptArrayPtr makes sense. But I don’t think we have an adopt function that is designed to work with fastMalloc’d memory. s/write/right/
(In reply to comment #6) > (From update of attachment 210538 [details]) > View in context: https://bugs.webkit.org/attachment.cgi?id=210538&action=review > > > Source/WTF/wtf/Vector.h:316 > > + return adoptPtr(buffer); > > This doesn’t seem write. If this was allocated with new, then adoptPtr makes sense, or if it was array new, then adoptArrayPtr makes sense. But I don’t think we have an adopt function that is designed to work with fastMalloc’d memory. Yeah this is completely wrong. What we need is a smart pointer for fastMalloced memory. (Or we could just use new/delete).