Re: [webkit-dev] Naming preference: SetForScope vs. TemporaryChange

[Maciej Stachowiak]

> On Dec 26, 2016, at 9:04 AM, Saam Barati  wrote:
> 
> Right. I see what you're saying. The name doesn't confuse me with respect to 
> these semantics but I see that's it's subtly wrong.
> 
> The use case I was thinking of is this:
> `
> class Foo {
>Foo() : m_change(someIntVariable, 20) { }
>...
>...
>ScopedChange m_change;
> };
> `
> 
> Which is admittedly an odd use case that probably doesn't exist in WebKit. 
> However, if this use case were common, the name ScopedChange feels wrong for 
> it. 

When you have a class intended for RAII-like use, making it also work as a data 
member or a heap object is a bit of a fool's errand. So I think it's ok to not 
account for this. If there was a sensible way to make a class only usable as 
local variable stack objects then this would be a great place to do it, though 
I don't think C++ has a good way to do that.

Regards,
Maciej

___
webkit-dev mailing list
webkit-dev@lists.webkit.org
https://lists.webkit.org/mailman/listinfo/webkit-dev

Re: [webkit-dev] Naming preference: SetForScope vs. TemporaryChange

[Saam Barati]

Right. I see what you're saying. The name doesn't confuse me with respect to 
these semantics but I see that's it's subtly wrong.

The use case I was thinking of is this:
`
class Foo {
Foo() : m_change(someIntVariable, 20) { }
...
...
ScopedChange m_change;
};
`

Which is admittedly an odd use case that probably doesn't exist in WebKit. 
However, if this use case were common, the name ScopedChange feels wrong for 
it. 

- Saam

> On Dec 25, 2016, at 7:29 PM, Maciej Stachowiak  wrote:
> 
> 
>> On Dec 25, 2016, at 11:35 AM, Saam Barati  wrote:
>> 
>> I like ScopedChange the most out of all the names. It's a bit unfortunate 
>> that such a name doesn't work well in the context of having a ScopedChange 
>> as a member variable. I think TemporaryChange works much better as a name if 
>> the use is as a member variable.
>> 
>> My hunch would be the most frequent use of this class is as a scoped change. 
>> If almost all of the uses are indeed for this, my name preference would be:
>> 1. ScopedChange
>> 2. TemporaryChange
>> 
>> If there are a lot of uses where the class isn't used as a scoped change, my 
>> preference would be to revert to TemporaryChange.
> 
> I'm not sure what distinction you are trying to draw between "scoped change" 
> and "temporary change". It seems pretty clear that this class is meant to be 
> used as a value object on the stack, so anything it does is scoped.
> 
> The distinction I was trying to draw is that, if you make additional changes 
> to the value still within the scope of the object, they will be overwritten. 
> That aspect isn't really captured by either "ScopedChange" or 
> "TemporaryChange". That's because, in some fundamental underlying sense, 
> what's really scoped is the restore of the original value, not the change. 
> 
> Maybe a concrete example would help show what I'm taking about:
> 
> // global
> double tachyonFlux = 3.14;
> 
> void redirectEnginesToShield()
> {
>ScopedChange(tachyonFlux, 2.0);
> 
>doOneThing();
> 
>tachyonFlux = 1.0;
> 
>doAnotherThing();
> 
>// on scope exit, both the scopedChange and the explicit assignment are 
> undone
> }
> 
> Perhaps this doesn't matter in practice because there's never actually 
> additional assignments in the scope of one of these things so it will be 
> clear enough as actually used.
> 
> Regards,
> Maciej
> 
> 
>> 
>> - Saam
>> 
>>> On Dec 23, 2016, at 6:50 AM, Maciej Stachowiak  wrote:
>>> 
>>> 
>>> A few more coats of paint for the bike shed:
>>> 
>>> It's a little unusual to have a class name that's a verb phrase instead of 
>>> a noun phrase. And in this case if you interpret "Set" as a noun you'll get 
>>> entirely the wrong idea. Some alternatives that avoid this, but has the 
>>> better clarity of "Scope" instead of "Temporary" would be "ScopedChange or 
>>> "ScopedAssignment".
>>> 
>>> One additional thing to think about: the class doesn't just have the effect 
>>> of limiting the assignment to a scope. It will also undo any further 
>>> assignments to the reference it holds that happen until it is destroyed. 
>>> Save-restore semantics like this are common but often the names involved 
>>> highlight the restore rather than the setting. I can't think of a great 
>>> name off the top of my head but something like RestoreOnScopeExit seems 
>>> more technically accurate than SetForScope.
>>> 
>>> - Maciej
>>> 
 On Dec 23, 2016, at 6:32 AM, Michael Catanzaro  
 wrote:
 
 On Fri, 2016-12-23 at 05:42 +, Yusuke SUZUKI wrote:
> Personally I like the name "SetForScope" since the name "scope"
> states that this value change is tied to C++ scope.
 
 Me too. The name is pretty clear. The first time I saw TemporaryChange
 I had to look at the implementation to see what it did.
 
 Michael
 ___
 webkit-dev mailing list
 webkit-dev@lists.webkit.org
 https://lists.webkit.org/mailman/listinfo/webkit-dev
>>> 
> 
___
webkit-dev mailing list
webkit-dev@lists.webkit.org
https://lists.webkit.org/mailman/listinfo/webkit-dev

Re: [webkit-dev] Naming preference: SetForScope vs. TemporaryChange

[Antti Koivisto]

On Mon, Dec 26, 2016 at 5:29 AM, Maciej Stachowiak  wrote:

>
> ScopedChange(tachyonFlux, 2.0);
>

I wish there was a compiler warning against this. I have caused at least
one bug by doing this exact thing.

(a temporary gets deleted at the end of the expression)


   antti
___
webkit-dev mailing list
webkit-dev@lists.webkit.org
https://lists.webkit.org/mailman/listinfo/webkit-dev

Re: [webkit-dev] Naming preference: SetForScope vs. TemporaryChange

[Maciej Stachowiak]

> On Dec 25, 2016, at 11:35 AM, Saam Barati  wrote:
> 
> I like ScopedChange the most out of all the names. It's a bit unfortunate 
> that such a name doesn't work well in the context of having a ScopedChange as 
> a member variable. I think TemporaryChange works much better as a name if the 
> use is as a member variable.
> 
> My hunch would be the most frequent use of this class is as a scoped change. 
> If almost all of the uses are indeed for this, my name preference would be:
> 1. ScopedChange
> 2. TemporaryChange
> 
> If there are a lot of uses where the class isn't used as a scoped change, my 
> preference would be to revert to TemporaryChange.

I'm not sure what distinction you are trying to draw between "scoped change" 
and "temporary change". It seems pretty clear that this class is meant to be 
used as a value object on the stack, so anything it does is scoped.

The distinction I was trying to draw is that, if you make additional changes to 
the value still within the scope of the object, they will be overwritten. That 
aspect isn't really captured by either "ScopedChange" or "TemporaryChange". 
That's because, in some fundamental underlying sense, what's really scoped is 
the restore of the original value, not the change. 

Maybe a concrete example would help show what I'm taking about:

// global
double tachyonFlux = 3.14;

void redirectEnginesToShield()
{
ScopedChange(tachyonFlux, 2.0);

doOneThing();

tachyonFlux = 1.0;

doAnotherThing();

// on scope exit, both the scopedChange and the explicit assignment are 
undone
}

Perhaps this doesn't matter in practice because there's never actually 
additional assignments in the scope of one of these things so it will be clear 
enough as actually used.

Regards,
Maciej


> 
> - Saam
> 
>> On Dec 23, 2016, at 6:50 AM, Maciej Stachowiak  wrote:
>> 
>> 
>> A few more coats of paint for the bike shed:
>> 
>> It's a little unusual to have a class name that's a verb phrase instead of a 
>> noun phrase. And in this case if you interpret "Set" as a noun you'll get 
>> entirely the wrong idea. Some alternatives that avoid this, but has the 
>> better clarity of "Scope" instead of "Temporary" would be "ScopedChange or 
>> "ScopedAssignment".
>> 
>> One additional thing to think about: the class doesn't just have the effect 
>> of limiting the assignment to a scope. It will also undo any further 
>> assignments to the reference it holds that happen until it is destroyed. 
>> Save-restore semantics like this are common but often the names involved 
>> highlight the restore rather than the setting. I can't think of a great name 
>> off the top of my head but something like RestoreOnScopeExit seems more 
>> technically accurate than SetForScope.
>> 
>> - Maciej
>> 
>>> On Dec 23, 2016, at 6:32 AM, Michael Catanzaro  
>>> wrote:
>>> 
>>> On Fri, 2016-12-23 at 05:42 +, Yusuke SUZUKI wrote:
 Personally I like the name "SetForScope" since the name "scope"
 states that this value change is tied to C++ scope.
>>> 
>>> Me too. The name is pretty clear. The first time I saw TemporaryChange
>>> I had to look at the implementation to see what it did.
>>> 
>>> Michael
>>> ___
>>> webkit-dev mailing list
>>> webkit-dev@lists.webkit.org
>>> https://lists.webkit.org/mailman/listinfo/webkit-dev
>> 

___
webkit-dev mailing list
webkit-dev@lists.webkit.org
https://lists.webkit.org/mailman/listinfo/webkit-dev

Re: [webkit-dev] Naming preference: SetForScope vs. TemporaryChange

[Saam Barati]

I like ScopedChange the most out of all the names. It's a bit unfortunate that 
such a name doesn't work well in the context of having a ScopedChange as a 
member variable. I think TemporaryChange works much better as a name if the use 
is as a member variable.

My hunch would be the most frequent use of this class is as a scoped change. If 
almost all of the uses are indeed for this, my name preference would be:
1. ScopedChange
2. TemporaryChange

If there are a lot of uses where the class isn't used as a scoped change, my 
preference would be to revert to TemporaryChange.

- Saam

> On Dec 23, 2016, at 6:50 AM, Maciej Stachowiak  wrote:
> 
> 
> A few more coats of paint for the bike shed:
> 
> It's a little unusual to have a class name that's a verb phrase instead of a 
> noun phrase. And in this case if you interpret "Set" as a noun you'll get 
> entirely the wrong idea. Some alternatives that avoid this, but has the 
> better clarity of "Scope" instead of "Temporary" would be "ScopedChange or 
> "ScopedAssignment".
> 
> One additional thing to think about: the class doesn't just have the effect 
> of limiting the assignment to a scope. It will also undo any further 
> assignments to the reference it holds that happen until it is destroyed. 
> Save-restore semantics like this are common but often the names involved 
> highlight the restore rather than the setting. I can't think of a great name 
> off the top of my head but something like RestoreOnScopeExit seems more 
> technically accurate than SetForScope.
> 
> - Maciej
> 
>> On Dec 23, 2016, at 6:32 AM, Michael Catanzaro  wrote:
>> 
>> On Fri, 2016-12-23 at 05:42 +, Yusuke SUZUKI wrote:
>>> Personally I like the name "SetForScope" since the name "scope"
>>> states that this value change is tied to C++ scope.
>> 
>> Me too. The name is pretty clear. The first time I saw TemporaryChange
>> I had to look at the implementation to see what it did.
>> 
>> Michael
>> ___
>> webkit-dev mailing list
>> webkit-dev@lists.webkit.org
>> https://lists.webkit.org/mailman/listinfo/webkit-dev
> 
___
webkit-dev mailing list
webkit-dev@lists.webkit.org
https://lists.webkit.org/mailman/listinfo/webkit-dev

Re: [webkit-dev] Naming preference: SetForScope vs. TemporaryChange

[Maciej Stachowiak]

A few more coats of paint for the bike shed:

It's a little unusual to have a class name that's a verb phrase instead of a 
noun phrase. And in this case if you interpret "Set" as a noun you'll get 
entirely the wrong idea. Some alternatives that avoid this, but has the better 
clarity of "Scope" instead of "Temporary" would be "ScopedChange or 
"ScopedAssignment".

One additional thing to think about: the class doesn't just have the effect of 
limiting the assignment to a scope. It will also undo any further assignments 
to the reference it holds that happen until it is destroyed. Save-restore 
semantics like this are common but often the names involved highlight the 
restore rather than the setting. I can't think of a great name off the top of 
my head but something like RestoreOnScopeExit seems more technically accurate 
than SetForScope.

 - Maciej

> On Dec 23, 2016, at 6:32 AM, Michael Catanzaro  wrote:
> 
> On Fri, 2016-12-23 at 05:42 +, Yusuke SUZUKI wrote:
>> Personally I like the name "SetForScope" since the name "scope"
>> states that this value change is tied to C++ scope.
> 
> Me too. The name is pretty clear. The first time I saw TemporaryChange
> I had to look at the implementation to see what it did.
> 
> Michael
> ___
> webkit-dev mailing list
> webkit-dev@lists.webkit.org
> https://lists.webkit.org/mailman/listinfo/webkit-dev

___
webkit-dev mailing list
webkit-dev@lists.webkit.org
https://lists.webkit.org/mailman/listinfo/webkit-dev

Re: [webkit-dev] Naming preference: SetForScope vs. TemporaryChange

[Michael Catanzaro]

On Fri, 2016-12-23 at 05:42 +, Yusuke SUZUKI wrote:
> Personally I like the name "SetForScope" since the name "scope"
> states that this value change is tied to C++ scope.

Me too. The name is pretty clear. The first time I saw TemporaryChange
I had to look at the implementation to see what it did.

Michael
___
webkit-dev mailing list
webkit-dev@lists.webkit.org
https://lists.webkit.org/mailman/listinfo/webkit-dev

Re: [webkit-dev] Naming preference: SetForScope vs. TemporaryChange

[Yusuke SUZUKI]

Personally I like the name "SetForScope" since the name "scope" states that
this value change is tied to C++ scope.

On Fri, Dec 23, 2016 at 11:32 JF Bastien  wrote:

> "Scope" seems to capture the C++ nature better.
>
> "Temporary" isn't clear on how long it'll last IMO.
>
>
> On Thu, Dec 22, 2016 at 6:28 PM Saam barati  wrote:
>
> Hi all,
>
> Recently, Yusuke found that JSC and WTF both had the exact same RAII
> helper data structure. JSC called it SetForScope, and WTF called it
> TemporaryChange. (Details here:
> https://bugs.webkit.org/show_bug.cgi?id=164761). Yusuke went to replace
> JSC’s use of SetForScope with TemporaryChange. When I reviewed his patch, I
> suggested to him to rename the class to SetForScope because it was my
> personal preference of the two names. However, I should have first
> discussed this change with other WebKit developers (so I’m doing that now,
> retroactively).
>
> If there is a strong preference to use the name TemporaryChange instead of
> SetForScope, I’ll rename the class back to its original name.
>
> My personal preference is still for the name SetForScope, but my feelings
> are not strongly tied to one name or the other.
>
> - Saam
> ___
>
> webkit-dev mailing list
>
> webkit-dev@lists.webkit.org
>
> https://lists.webkit.org/mailman/listinfo/webkit-dev
>
>
>
> ___
>
> webkit-dev mailing list
>
> webkit-dev@lists.webkit.org
>
> https://lists.webkit.org/mailman/listinfo/webkit-dev
>
>
___
webkit-dev mailing list
webkit-dev@lists.webkit.org
https://lists.webkit.org/mailman/listinfo/webkit-dev

Re: [webkit-dev] Naming preference: SetForScope vs. TemporaryChange

[JF Bastien]

"Scope" seems to capture the C++ nature better.

"Temporary" isn't clear on how long it'll last IMO.


On Thu, Dec 22, 2016 at 6:28 PM Saam barati  wrote:

> Hi all,
>
> Recently, Yusuke found that JSC and WTF both had the exact same RAII
> helper data structure. JSC called it SetForScope, and WTF called it
> TemporaryChange. (Details here:
> https://bugs.webkit.org/show_bug.cgi?id=164761). Yusuke went to replace
> JSC’s use of SetForScope with TemporaryChange. When I reviewed his patch, I
> suggested to him to rename the class to SetForScope because it was my
> personal preference of the two names. However, I should have first
> discussed this change with other WebKit developers (so I’m doing that now,
> retroactively).
>
> If there is a strong preference to use the name TemporaryChange instead of
> SetForScope, I’ll rename the class back to its original name.
>
> My personal preference is still for the name SetForScope, but my feelings
> are not strongly tied to one name or the other.
>
> - Saam
> ___
>
> webkit-dev mailing list
>
> webkit-dev@lists.webkit.org
>
> https://lists.webkit.org/mailman/listinfo/webkit-dev
>
>
___
webkit-dev mailing list
webkit-dev@lists.webkit.org
https://lists.webkit.org/mailman/listinfo/webkit-dev