Bug 157591 – Protector Ref/RefPtrs should have a specified naming style

• Component: WebKit Website
• Status: NEW

[Brady Eidson]

Protector Ref/RefPtrs should have a specified naming style

This came up during patch review of https://bugs.webkit.org/show_bug.cgi?id=157448 but has also come up many times before.

The name for a Ref/RefPtr protector should be "protectedVariableName(variableName)"

Proper naming for a protector should be like the following examples:

RefPtr<Node> protectedThis(this);
Ref<Node> protectedThis(*this);
RefPtr<Node> protectedNode(node);
RefPtr<Connection> protectedOtherConnection(&otherConnection);
RefPtr<Widget> protectedWidget(m_widget);

Improper naming for which there are many examples in WebCore:

RefPtr<Node> protect(this);
RefPtr<Node> protector(this);
RefPtr<Node> self(this);
RefPtr<Element> elementRef(this);
RefPtr<Tree> someWeirdoOtherName(tree);

[Brady Eidson]

I have the rules implemented in style checker, and am working on a patch to the website as well.

Once I'm done and post the patch, I will email webkit-dev for comment.

[Brady Eidson]

Created attachment 278676 [details]
Patch

[Simon Fraser (smfr)]

http://lxr.mozilla.org/mozilla-central/search?string=kungfudeathgrip

[Brady Eidson]

(In reply to comment #3)
> http://lxr.mozilla.org/mozilla-central/search?string=kungfudeathgrip

That's hilarious.

And, no.

[Michael Catanzaro]

(In reply to comment #0)
> Proper naming for a protector should be like the following examples:
> 
> RefPtr<Node> protectedThis(this);
> Ref<Node> protectedThis(*this);
> RefPtr<Node> protectedNode(node);
> RefPtr<Connection> protectedOtherConnection(&otherConnection);
> RefPtr<Widget> protectedWidget(m_widget);

FWIW I think this is a great idea.

I like "protector" as well, but that name does not indicate the original use of the variable.

[Darin Adler]

Comment on attachment 278676 [details]
Patch

Lets do this. Hope anyone who doesn’t like this style had a chance to complain.

[Brady Eidson]

(In reply to comment #6)
> Comment on attachment 278676 [details]
> Patch
> 
> Lets do this. Hope anyone who doesn’t like this style had a chance to
> complain.

Combine the bug with IRC and I've only heard positive.

webkit-dev has silence in either direction.

I'll give potential naysayers overnight to speak up before landing this in the office tomorrow morning.

[Ryosuke Niwa]

Comment on attachment 278676 [details]
Patch

View in context: https://bugs.webkit.org/attachment.cgi?id=278676&action=review

> Websites/webkit.org/code-style.md:723
> +RefPtr<Element> protectedElement(&element);

Having to repeat Element/element three times in a single line seems overly redundant.
How about protection(this) or protector(this)?

[Antti Koivisto]

I agree with Ryosuke. How is including the variable name to the protector name useful? The variable you are protecting is right there and you never use the protector afterwards. Our coding style generally avoids this sort of redundancy.

The cases where you are protecting multiple things in a single function are (or should be) rare.

I like 'protect' since it reads well but 'protected' is fine too.

[Brady Eidson]

Since there's now stated opposition, I'll wait until there's agreement before landing.

---

(In reply to comment #8)
> > Websites/webkit.org/code-style.md:723
> > +RefPtr<Element> protectedElement(&element);
> 
> Having to repeat Element/element three times in a single line seems overly
> redundant.

Repeating Element 3 times only comes up in certain cases, such as when you're protecting an Element object named element.

While that is common, quite commonly we also see the variable name being protected *not* match the class it is a member of.

A handful of corrections to existing WebCore code would include:
RefPtr<Node> protectedRoot(root);
RefPtr<SharedBuffer> protectedData(data);
RefPtr<Node> protectedOrigin(origin);

I personally do not find the redundancy to be off-putting, but agree that - as a matter of taste - some might. However, it also has value, because...

(In reply to comment #9)
> I agree with Ryosuke. How is including the variable name to the protector
> name useful? The variable you are protecting is right there and you never
> use the protector afterwards. 

While doing a global replace just for the (this) protectors, I found there are many cases where the protector is used later in the function. 

In some of these cases the function could be cleaned up to use the original variable instead, but in others it cannot. Examples include when the protector is captured in a lambda or when the original variable has been WTFMove()'d to somewhere else.

So "you never use the protector afterwards" is a flawed argument against the style.

> The cases where you are protecting multiple things in a single function are
> (or should be) rare.

Again, while doing the global replace just for the (this) protectors, I did come across a non-trivial number of places where there were multiple protectors.

---

If I hadn't run the new check-webkit-style script against WebCore, I might've been inclined to agree with this line of complaint against the proposed style.

But that's actually why I went ahead and implemented the script just to see how simple things are. I believe there are enough instances of complexity that this new style does make sense.

[Michael Catanzaro]

(In reply to comment #9)
> I like 'protect' since it reads well but 'protected' is fine too.

I would prefer the noun ("protector") rather than the verb ("protect") or lone adjective ("protected" on its own).

[Brady Eidson]

(In reply to comment #11)
> (In reply to comment #9)
> > I like 'protect' since it reads well but 'protected' is fine too.
> 
> I would prefer the noun ("protector") rather than the verb ("protect") or
> lone adjective ("protected" on its own).

If we *do* decide to not go forward with the "protectedFoo" style, I also strongly prefer "protector", because it is a noun.

That said I still support the full proposal here.

[Antti Koivisto]

Agreeing on a common name (protect/protector/protected/whatever) is a good idea and I'm fine with any choice. The mandatory variable name match seems unnecessary in many/most cases.

[Brady Eidson]

(In reply to comment #13)
> Agreeing on a common name (protect/protector/protected/whatever) is a good
> idea and I'm fine with any choice. The mandatory variable name match seems
> unnecessary in many/most cases.

For the cases (that do exist) where there are multiple protectors, what would you suggest for resolving them?

[Darin Adler]

To repeat what Brady said:

I originally started using the name protectedXXX when I realized that it was useful to capture the protected variable, not the original raw one, in a lambda defined in the function. A pure protector that only extends the lifetime but is never used was common, but in a way those are the worst kind because they are so hard to reason about. Even when we capture both “this” and “protectedThis” because we want to use the implicit “this->” to make the lambda look cleaner, it’s much clearer to capture “protectedThis” rather than “protector”.

Once I was using this in the cases where I wanted to capture, I realized that I found the naming scheme clearer in all those other cases, which is why I slightly prefer it.

Another way to put it is that I don’t think we could say it would be good style to use “protector” everywhere; it’s not a good name for a RefPtr that we capture in a lambda. So we’d need to avoid the “protector” name for those cases. And we’d need more than one name then.

[Antti Koivisto]

I definitely support using more specific name where it is needed or helpful (and not just in this context).

[Brady Eidson]

Perhaps I could propose a two point compromise:
-Protectors for "this" should always be called "protectedThis"
-Protectors for other variables can either be called "protector" or "protectedVariableName", but nothing else.

I would be happy with this proposal.

[Ryosuke Niwa]

(In reply to comment #17)
> Perhaps I could propose a two point compromise:
> -Protectors for "this" should always be called "protectedThis"
> -Protectors for other variables can either be called "protector" or
> "protectedVariableName", but nothing else.
> 
> I would be happy with this proposal.

That seems like a nice compromise.

[Brady Eidson]

Created attachment 278769 [details]
Updated patch

Made the changes to the regex to treat "this" and non-this differently, as well as combined it down to a single regex.

Also made the changes to the website.

Leaving Darin as the reviewer for now, since the changes are pretty straightforward.

Will be looking to land this relatively soonish unless somebody chimes in as being unhappy with the compromise.

[Brady Eidson]

Created attachment 278770 [details]
Updated patch

[Brady Eidson]

Landed in http://trac.webkit.org/changeset/200859

[Saam Barati]

(In reply to comment #17)
> Perhaps I could propose a two point compromise:
> -Protectors for "this" should always be called "protectedThis"
> -Protectors for other variables can either be called "protector" or
> "protectedVariableName", but nothing else.
> 
> I would be happy with this proposal.

My two cents even though this patch has already landed:

I like the rule that says always "protectedVariableName" over "protector" because it's a simple rule to follow. It may be slightly annoying/redundant to have protectedVariableName in contexts where the "variableName" part isn't needed. But it will be even more annoying/confusing to see just "protector" when "protectedVariableName" is more appropriate. Having "protectedVariableName" be the only style allowed means we will never run into the latter code.

[Darin Adler]

(In reply to comment #22)
> I like the rule that says always "protectedVariableName" over "protector"
> because it's a simple rule to follow.

Me too.

[Antti Koivisto]

Style checker is now demanding bad naming, see https://bugs.webkit.org/show_bug.cgi?id=157694#c9 I don't think we should keep it like this.

Also I'm not sure expanding the protector 'pattern' to cover lambdas with uniform naming is useful in the first place. For that something like 'strongFoo', 'strongThis' makes more sense to me (and is already used in a number of places), or just using our usual context dependent descriptive names.

[Darin Adler]

I’m not sure this guideline fits into the style checker well. Ref and RefPtr are normal types that are used in many normal ways that are not the “protected” idiom. How would the style checker know?

[Brady Eidson]

(In reply to comment #24)
> Style checker is now demanding bad naming, see
> https://bugs.webkit.org/show_bug.cgi?id=157694#c9 I don't think we should
> keep it like this.

I don't think anybody ever argued that we should keep bad checks in, and I would love to stay on top of the impl of the rule to see how adept it can be made.

That said, in this case, it's bad RefPtr use - The method protects the RefCounted argument but doesn't need to.

The local RefPtr actually causes unneeded Ref churn.

In fact, same thing on line 94 of the same file inside CachedFont::ensureCustomFontData - Local RefPtr that looks like it might serve a use as a protector or "other", but is actually useless.

> Also I'm not sure expanding the protector 'pattern' to cover lambdas with
> uniform naming is useful in the first place. For that something like
> 'strongFoo', 'strongThis' makes more sense to me (and is already used in a
> number of places), or just using our usual context dependent descriptive
> names.

Actually, this is exactly where Darin first started using the "protectedThis" pattern, I first started noticing in his reviews, and I grew to agree with him.

I understand that this particular use is subjective and a matter of taste.

But, objectively, "strongThis" and "protectedThis" are dangerous names to conflate, as there's a very important type of smart pointer called "JSC::Strong"

[Brady Eidson]

(In reply to comment #26)
> (In reply to comment #24)
> > Style checker is now demanding bad naming, see
> > https://bugs.webkit.org/show_bug.cgi?id=157694#c9 I don't think we should
> > keep it like this.
> 
> I don't think anybody ever argued that we should keep bad checks in, and I
> would love to stay on top of the impl of the rule to see how adept it can be
> made.
> 
> That said, in this case, it's bad RefPtr use - The method protects the
> RefCounted argument but doesn't need to.
> 
> The local RefPtr actually causes unneeded Ref churn.
> 
> In fact, same thing on line 94 of the same file inside
> CachedFont::ensureCustomFontData - Local RefPtr that looks like it might
> serve a use as a protector or "other", but is actually useless.

I'm chewing on ways that the message for the rule and perhaps the strictness level can be adjusted to suggest that the detected line may actually just be a misuse of RefPtr.

[Brady Eidson]

(In reply to comment #25)
> I’m not sure this guideline fits into the style checker well. Ref and RefPtr
> are normal types that are used in many normal ways that are not the
> “protected” idiom. How would the style checker know?

I agree that parsing line-by-line, as the style checker currently does, it would be difficult to know.

If the style checker was aware of larger scopes the rule could much more effectively be implemented.

Has there ever been discussion/effort to make the style checker leverage clang/llvm somehow? (I'm just pulling this out of thin air, am unsure how feasible it would be)

[Brady Eidson]

(In reply to comment #25)
> I’m not sure this guideline fits into the style checker well. Ref and RefPtr
> are normal types that are used in many normal ways that are not the
> “protected” idiom. How would the style checker know?

One way that we could do this today with line-by-line style checking would be to manually check for the known *bad* names. protect, protected, strong, nameRef, etc.

[Brady Eidson]

(In reply to comment #26)
> (In reply to comment #24)
> > Style checker is now demanding bad naming, see
> > https://bugs.webkit.org/show_bug.cgi?id=157694#c9 I don't think we should
> > keep it like this.
> 
> I don't think anybody ever argued that we should keep bad checks in, and I
> would love to stay on top of the impl of the rule to see how adept it can be
> made.
> 
> That said, in this case, it's bad RefPtr use - The method protects the
> RefCounted argument but doesn't need to.

Never mind - The way the method is constructed, it is valid RefPtr use, as the RefPtr is also used to hold a SharedBuffer::adoptVector;