If we allow Ref to become "invalid" after moving out of it, we don't need the PassRef class at all. Ref<T> for return values, Ref<T>&& for arguments.
Created attachment 243199 [details] Snack for EWS
Created attachment 243200 [details] Snack for EWS II
Attachment 243200 [details] did not pass style-queue: ERROR: Source/WTF/wtf/PassRefPtr.h:29: Code inside a namespace should not be indented. [whitespace/indent] [4] ERROR: Source/WebCore/platform/graphics/Font.cpp:218: Should be indented on a separate line, with the colon or comma first on that line. [whitespace/indent] [4] ERROR: Source/WebCore/rendering/style/DataRef.h:33: Should be indented on a separate line, with the colon or comma first on that line. [whitespace/indent] [4] ERROR: Source/WTF/wtf/Ref.h:104: This { should be at the end of the previous line [whitespace/braces] [4] ERROR: Source/WTF/wtf/Ref.h:109: This { should be at the end of the previous line [whitespace/braces] [4] ERROR: Source/WTF/wtf/Ref.h:120: Should be indented on a separate line, with the colon or comma first on that line. [whitespace/indent] [4] ERROR: Source/WTF/wtf/text/WTFString.h:118: Should be indented on a separate line, with the colon or comma first on that line. [whitespace/indent] [4] Total errors found: 7 in 582 files If any of these errors are false positives, please file a bug against check-webkit-style.
Created attachment 243207 [details] Potentially a patch
Attachment 243207 [details] did not pass style-queue: ERROR: Source/WTF/wtf/PassRefPtr.h:29: Code inside a namespace should not be indented. [whitespace/indent] [4] ERROR: Source/WebCore/rendering/style/DataRef.h:33: Should be indented on a separate line, with the colon or comma first on that line. [whitespace/indent] [4] ERROR: Source/WTF/wtf/Ref.h:104: This { should be at the end of the previous line [whitespace/braces] [4] ERROR: Source/WTF/wtf/Ref.h:109: This { should be at the end of the previous line [whitespace/braces] [4] ERROR: Source/WTF/wtf/text/WTFString.h:118: Should be indented on a separate line, with the colon or comma first on that line. [whitespace/indent] [4] Total errors found: 5 in 599 files If any of these errors are false positives, please file a bug against check-webkit-style.
Created attachment 243220 [details] Patch
Attachment 243220 [details] did not pass style-queue: ERROR: Source/WTF/wtf/PassRefPtr.h:29: Code inside a namespace should not be indented. [whitespace/indent] [4] ERROR: Source/WebCore/rendering/style/DataRef.h:33: Should be indented on a separate line, with the colon or comma first on that line. [whitespace/indent] [4] ERROR: Source/WTF/wtf/Ref.h:105: This { should be at the end of the previous line [whitespace/braces] [4] ERROR: Source/WTF/wtf/Ref.h:110: This { should be at the end of the previous line [whitespace/braces] [4] ERROR: Source/WTF/wtf/text/WTFString.h:118: Should be indented on a separate line, with the colon or comma first on that line. [whitespace/indent] [4] Total errors found: 5 in 599 files If any of these errors are false positives, please file a bug against check-webkit-style.
Comment on attachment 243220 [details] Patch View in context: https://bugs.webkit.org/attachment.cgi?id=243220&action=review Nice. If we forget to use WTF::move when passing a Ref to a Ref&& argument, what happens? Do we get a compilation error? I’m also a bit confused about why we have to use WTF::move to when assigning to a Ref from a Ref&& argument. What does the WTF::move accomplish again? Same question but about the case where we are passing a Ref&& argument into another function that takes a Ref&&. I think we have a *lot* of WTF::move that we could remove now that we are using Ref&& instead of PassRef, unless I’m mixed up about something. > Source/WTF/wtf/text/WTFString.h:118 > - String(PassRef<StringImpl>&& impl) : m_impl(std::forward<PassRef<StringImpl>>(impl)) { } > + String(Ref<StringImpl>&& impl) : m_impl(std::forward<Ref<StringImpl>>(impl)) { } I would have used WTF::move here. Is std::forward better for some reason? I was under the impression that std::forward typically only was needed when there were templates involved and the precise type was not known. > Source/WTF/wtf/text/WTFString.h:515 > - : m_impl(vector.size() ? StringImpl::create(vector.data(), vector.size()) : *StringImpl::empty()) > + : m_impl(vector.size() ? StringImpl::create(vector.data(), vector.size()) : Ref<StringImpl>(*StringImpl::empty())) Explicit construction here is annoying; I see it’s needed in a few places. More on this in a comment below. > Source/WebCore/css/CSSFunctionValue.h:43 > + static Ref<CSSFunctionValue> create(String name, PassRefPtr<CSSValueList> args) Peculiar that this uses String. I would have expected const String& and I was wondering if String is used whether we also should be doing WTF::move on the String when passing it along. > Source/WebCore/platform/graphics/Font.cpp:221 > + FontGlyphsCacheEntry(FontGlyphsCacheKey&& k, Ref<FontGlyphs>&& g) > + : key(WTF::move(k)) > + , glyphs(WTF::move(g)) > + { } We should just dump constructors like this from structs and use FontGlyphsCacheEntry{ x, y } syntax instead of a constructor. > Source/WebCore/rendering/RenderScrollbarPart.h:38 > + RenderScrollbarPart(Document&, Ref<RenderStyle>&&, RenderScrollbar* = 0, ScrollbarPart = NoPart); Missed a chance to nullptr-ify here. > Source/WebKit/mac/WebCoreSupport/WebEditorClient.mm:258 > - Ref<MutableStyleProperties> mutableStyle(style->isMutable() ? static_cast<MutableStyleProperties&>(*style) : style->mutableCopy()); > + Ref<MutableStyleProperties> mutableStyle(style->isMutable() ? Ref<MutableStyleProperties>(static_cast<MutableStyleProperties&>(*style)) : style->mutableCopy()); We were getting away without the cast before. Why do we need it now? If this was going to happen a lot, I would suggest providing a template function to wrap in a Ref like this, so we don’t have to repeat the type names such as MutableStyleProperties.
Comment on attachment 243220 [details] Patch Clearing flags on attachment: 243220 Committed r177259: <http://trac.webkit.org/changeset/177259>
All reviewed patches have been landed. Closing bug.
(In reply to comment #8) > Comment on attachment 243220 [details] > Patch > > View in context: > https://bugs.webkit.org/attachment.cgi?id=243220&action=review > > Nice. > > If we forget to use WTF::move when passing a Ref to a Ref&& argument, what > happens? Do we get a compilation error? We do. You either have to WTF::move(), or .copyRef() the Ref, in order to pass it to a Ref&& argument. > I’m also a bit confused about why we have to use WTF::move to when assigning > to a Ref from a Ref&& argument. What does the WTF::move accomplish again? > Same question but about the case where we are passing a Ref&& argument into > another function that takes a Ref&&. I think we have a *lot* of WTF::move > that we could remove now that we are using Ref&& instead of PassRef, unless > I’m mixed up about something. AFAICT that is still necessary. > > Source/WTF/wtf/text/WTFString.h:118 > > - String(PassRef<StringImpl>&& impl) : m_impl(std::forward<PassRef<StringImpl>>(impl)) { } > > + String(Ref<StringImpl>&& impl) : m_impl(std::forward<Ref<StringImpl>>(impl)) { } > > I would have used WTF::move here. Is std::forward better for some reason? I > was under the impression that std::forward typically only was needed when > there were templates involved and the precise type was not known. > > > Source/WebCore/css/CSSFunctionValue.h:43 > > + static Ref<CSSFunctionValue> create(String name, PassRefPtr<CSSValueList> args) > > Peculiar that this uses String. I would have expected const String& and I > was wondering if String is used whether we also should be doing WTF::move on > the String when passing it along. Will tweak. > > Source/WebCore/platform/graphics/Font.cpp:221 > > + FontGlyphsCacheEntry(FontGlyphsCacheKey&& k, Ref<FontGlyphs>&& g) > > + : key(WTF::move(k)) > > + , glyphs(WTF::move(g)) > > + { } > > We should just dump constructors like this from structs and use > FontGlyphsCacheEntry{ x, y } syntax instead of a constructor. Will tweak. > > Source/WebCore/rendering/RenderScrollbarPart.h:38 > > + RenderScrollbarPart(Document&, Ref<RenderStyle>&&, RenderScrollbar* = 0, ScrollbarPart = NoPart); > > Missed a chance to nullptr-ify here. Will tweak. > > > Source/WebKit/mac/WebCoreSupport/WebEditorClient.mm:258 > > - Ref<MutableStyleProperties> mutableStyle(style->isMutable() ? static_cast<MutableStyleProperties&>(*style) : style->mutableCopy()); > > + Ref<MutableStyleProperties> mutableStyle(style->isMutable() ? Ref<MutableStyleProperties>(static_cast<MutableStyleProperties&>(*style)) : style->mutableCopy()); > > We were getting away without the cast before. Why do we need it now? Because Chris added an operator T& to Ref, so now that ternary becomes ambiguous. > > If this was going to happen a lot, I would suggest providing a template > function to wrap in a Ref like this, so we don’t have to repeat the type > names such as MutableStyleProperties. I don't think it'll happen a lot, though that's not a bad idea either.
(In reply to comment #11) > (In reply to comment #8) > > Comment on attachment 243220 [details] > > Patch > > > > View in context: > > https://bugs.webkit.org/attachment.cgi?id=243220&action=review > > > > Nice. > > > > If we forget to use WTF::move when passing a Ref to a Ref&& argument, what > > happens? Do we get a compilation error? > > We do. You either have to WTF::move(), or .copyRef() the Ref, in order to > pass it to a Ref&& argument. > > > I’m also a bit confused about why we have to use WTF::move to when assigning > > to a Ref from a Ref&& argument. What does the WTF::move accomplish again? > > Same question but about the case where we are passing a Ref&& argument into > > another function that takes a Ref&&. I think we have a *lot* of WTF::move > > that we could remove now that we are using Ref&& instead of PassRef, unless > > I’m mixed up about something. > > AFAICT that is still necessary. Anything that has a name is considered an lvalue, even if it's an rvalue reference. > > > > Source/WTF/wtf/text/WTFString.h:118 > > > - String(PassRef<StringImpl>&& impl) : m_impl(std::forward<PassRef<StringImpl>>(impl)) { } > > > + String(Ref<StringImpl>&& impl) : m_impl(std::forward<Ref<StringImpl>>(impl)) { } > > > > I would have used WTF::move here. Is std::forward better for some reason? I > > was under the impression that std::forward typically only was needed when > > there were templates involved and the precise type was not known. Yes, WTF::forward should only be used when the parameter is a "forwarding reference". (T&& where T is a template parameter).