Bug 157591 - Protector Ref/RefPtrs should have a specified naming style
Summary: Protector Ref/RefPtrs should have a specified naming style
Status: NEW
Alias: None
Product: WebKit
Classification: Unclassified
Component: WebKit Website (show other bugs)
Version: WebKit Nightly Build
Hardware: Unspecified Unspecified
: P2 Normal
Assignee: Brady Eidson
URL:
Keywords:
Depends on:
Blocks:
 
Reported: 2016-05-11 15:42 PDT by Brady Eidson
Modified: 2016-05-16 08:45 PDT (History)
11 users (show)

See Also:


Attachments
Patch (9.09 KB, patch)
2016-05-11 16:00 PDT, Brady Eidson
darin: review+
Details | Formatted Diff | Diff
Updated patch (9.80 KB, patch)
2016-05-12 15:25 PDT, Brady Eidson
no flags Details | Formatted Diff | Diff
Updated patch (9.83 KB, patch)
2016-05-12 15:28 PDT, Brady Eidson
no flags Details | Formatted Diff | Diff

Note You need to log in before you can comment on or make changes to this bug.
Description Brady Eidson 2016-05-11 15:42:25 PDT
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);
Comment 1 Brady Eidson 2016-05-11 15:43:27 PDT
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.
Comment 2 Brady Eidson 2016-05-11 16:00:36 PDT
Created attachment 278676 [details]
Patch
Comment 4 Brady Eidson 2016-05-11 17:14:40 PDT
(In reply to comment #3)
> http://lxr.mozilla.org/mozilla-central/search?string=kungfudeathgrip

That's hilarious.

And, no.
Comment 5 Michael Catanzaro 2016-05-11 20:33:54 PDT
(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.
Comment 6 Darin Adler 2016-05-11 21:50:43 PDT
Comment on attachment 278676 [details]
Patch

Lets do this. Hope anyone who doesn’t like this style had a chance to complain.
Comment 7 Brady Eidson 2016-05-11 21:57:07 PDT
(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.
Comment 8 Ryosuke Niwa 2016-05-11 23:50:04 PDT
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)?
Comment 9 Antti Koivisto 2016-05-12 02:38:15 PDT
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.
Comment 10 Brady Eidson 2016-05-12 06:35:55 PDT
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.
Comment 11 Michael Catanzaro 2016-05-12 07:52:03 PDT
(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).
Comment 12 Brady Eidson 2016-05-12 08:42:05 PDT
(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.
Comment 13 Antti Koivisto 2016-05-12 09:32:25 PDT
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.
Comment 14 Brady Eidson 2016-05-12 09:35:12 PDT
(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?
Comment 15 Darin Adler 2016-05-12 09:45:33 PDT
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.
Comment 16 Antti Koivisto 2016-05-12 09:55:09 PDT
I definitely support using more specific name where it is needed or helpful (and not just in this context).
Comment 17 Brady Eidson 2016-05-12 10:16:04 PDT
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.
Comment 18 Ryosuke Niwa 2016-05-12 13:56:15 PDT
(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.
Comment 19 Brady Eidson 2016-05-12 15:25:20 PDT
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.
Comment 20 Brady Eidson 2016-05-12 15:28:21 PDT
Created attachment 278770 [details]
Updated patch
Comment 21 Brady Eidson 2016-05-13 09:16:51 PDT
Landed in http://trac.webkit.org/changeset/200859
Comment 22 Saam Barati 2016-05-14 11:36:08 PDT
(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.
Comment 23 Darin Adler 2016-05-15 14:04:35 PDT
(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.
Comment 24 Antti Koivisto 2016-05-15 15:57:42 PDT
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.
Comment 25 Darin Adler 2016-05-15 16:02:54 PDT
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?
Comment 26 Brady Eidson 2016-05-16 08:32:34 PDT
(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"
Comment 27 Brady Eidson 2016-05-16 08:34:42 PDT
(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.
Comment 28 Brady Eidson 2016-05-16 08:37:33 PDT
(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)
Comment 29 Brady Eidson 2016-05-16 08:41:47 PDT
(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.
Comment 30 Brady Eidson 2016-05-16 08:45:48 PDT
(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;