rdar://85899879
Created attachment 448082 [details] For EWS
Comment on attachment 448082 [details] For EWS View in context: https://bugs.webkit.org/attachment.cgi?id=448082&action=review > Source/WebCore/html/HTMLAttachmentElement.h:67 > + RefPtr<HTMLImageElement> enclosingImageElement() const; Someone has to write down our rules about RefPtr vs. raw pointer in new code. It seems clear this should be RefPtr in case we call any function using it as a this pointer, but not sure where we wrote that down.
Comment on attachment 448082 [details] For EWS View in context: https://bugs.webkit.org/attachment.cgi?id=448082&action=review Thanks for the review! >> Source/WebCore/html/HTMLAttachmentElement.h:67 >> + RefPtr<HTMLImageElement> enclosingImageElement() const; > > Someone has to write down our rules about RefPtr vs. raw pointer in new code. It seems clear this should be RefPtr in case we call any function using it as a this pointer, but not sure where we wrote that down. +1 to this! I agree it would be great to have explicit guidelines around when to use RefPtr/Ref in the return type, vs. using RefPtr/Ref at the call site. My understanding was that we should generally return RefPtr/Ref instead of raw pointers/references and use `auto` in call sites, unless there's a good reason to avoid the ref-churn (e.g. performance). That said, I do still tend to pass back raw pointers or references sometimes for methods that are just simple getters around a ref-counted object :P.
Committed r287494 (245629@main): <https://commits.webkit.org/245629@main> All reviewed patches have been landed. Closing bug and clearing flags on attachment 448082 [details].
Comment on attachment 448082 [details] For EWS View in context: https://bugs.webkit.org/attachment.cgi?id=448082&action=review >>> Source/WebCore/html/HTMLAttachmentElement.h:67 >>> + RefPtr<HTMLImageElement> enclosingImageElement() const; >> >> Someone has to write down our rules about RefPtr vs. raw pointer in new code. It seems clear this should be RefPtr in case we call any function using it as a this pointer, but not sure where we wrote that down. > > +1 to this! I agree it would be great to have explicit guidelines around when to use RefPtr/Ref in the return type, vs. using RefPtr/Ref at the call site. > > My understanding was that we should generally return RefPtr/Ref instead of raw pointers/references and use `auto` in call sites, unless there's a good reason to avoid the ref-churn (e.g. performance). That said, I do still tend to pass back raw pointers or references sometimes for methods that are just simple getters around a ref-counted object :P. My understanding was that we should use RefPtr/Ref for local variables and not for argument types or return types. But I am not sure that rule works as a general rule, given that a return type has *some* of the same properties as a local variable, when used as an argument to a function or to make a member function call. I’m sure the full answer depends on the lifetime of the returned raw pointer, but it’s hard to reason about that given side effects. Like returning something owned by "this", but then a new value is assigned during the lifetime of that return value. And: a rule that contains the phrase "unless there’s a good reason" is difficult to follow, because of how subjective that is. You mention above that we should use a RefPtr return type and use "auto" at call sites, but in this patch we use a RefPtr return type and use "RefPtr" at call sites.
(In reply to Darin Adler from comment #5) > Comment on attachment 448082 [details] > For EWS > > View in context: > https://bugs.webkit.org/attachment.cgi?id=448082&action=review > > >>> Source/WebCore/html/HTMLAttachmentElement.h:67 > >>> + RefPtr<HTMLImageElement> enclosingImageElement() const; > >> > >> Someone has to write down our rules about RefPtr vs. raw pointer in new code. It seems clear this should be RefPtr in case we call any function using it as a this pointer, but not sure where we wrote that down. > > > > +1 to this! I agree it would be great to have explicit guidelines around when to use RefPtr/Ref in the return type, vs. using RefPtr/Ref at the call site. > > > > My understanding was that we should generally return RefPtr/Ref instead of raw pointers/references and use `auto` in call sites, unless there's a good reason to avoid the ref-churn (e.g. performance). That said, I do still tend to pass back raw pointers or references sometimes for methods that are just simple getters around a ref-counted object :P. > > My understanding was that we should use RefPtr/Ref for local variables and > not for argument types or return types. But I am not sure that rule works as > a general rule, given that a return type has *some* of the same properties > as a local variable, when used as an argument to a function or to make a > member function call. I’m sure the full answer depends on the lifetime of > the returned raw pointer, but it’s hard to reason about that given side > effects. Like returning something owned by "this", but then a new value is > assigned during the lifetime of that return value. I see — it sounds like this method's return type should really just be a raw pointer, then? (Especially since I already explicitly put it in a RefPtr, as you noted below). > > And: a rule that contains the phrase "unless there’s a good reason" is > difficult to follow, because of how subjective that is. > > You mention above that we should use a RefPtr return type and use "auto" at > call sites, but in this patch we use a RefPtr return type and use "RefPtr" > at call sites. Whoops, good catch — that's probably from when I was going back and forth when trying to decide between returning a raw pointer vs. RefPtr :P.
Reopening to attach new patch.
Created attachment 448155 [details] Return pointer instead of RefPtr
Comment on attachment 448155 [details] Return pointer instead of RefPtr View in context: https://bugs.webkit.org/attachment.cgi?id=448155&action=review > Source/WebCore/ChangeLog:10 > + Make a helper method on HTMLAttachmentElement return a raw pointer instead of a RefPtr. All call sites that > + store the result of this method in a local variable already use RefPtr. I know I created the doubt here, but I am still not sure about this; if any caller wanted to call a function on the result of this, then I think we’d be happier with RefPtr.
(In reply to Darin Adler from comment #9) > Comment on attachment 448155 [details] > Return pointer instead of RefPtr > > View in context: > https://bugs.webkit.org/attachment.cgi?id=448155&action=review > > > Source/WebCore/ChangeLog:10 > > + Make a helper method on HTMLAttachmentElement return a raw pointer instead of a RefPtr. All call sites that > > + store the result of this method in a local variable already use RefPtr. > > I know I created the doubt here, but I am still not sure about this; if any > caller wanted to call a function on the result of this, then I think we’d be > happier with RefPtr. Hm...fair enough. Maybe I'll just leave the return value as `RefPtr` but switch the `RefPtr` => `auto` at the call sites, then?
Created attachment 448158 [details] Use auto in a few more places
Committed r287521 (245656@main): <https://commits.webkit.org/245656@main> All reviewed patches have been landed. Closing bug and clearing flags on attachment 448158 [details].