Bug 73371 – Add a way to revert a variable to its previous value after leaving a scope.

• Component: Web Template Framework
• Status: RESOLVED FIXED

[David Levin]

Add a way to revert a variable to its previous value after leaving a scope.

[David Levin]

Created attachment 117063 [details]
Patch

[Adam Barth]

Comment on attachment 117063 [details]
Patch

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

> Source/JavaScriptCore/wtf/AutoRevert.h:39
> +// shorter lifetime than its scoped_variable, to prevent invalid memory writes

scoped_variable => scopedVariable

> Tools/TestWebKitAPI/Tests/WTF/AutoRevert.cpp:36
> +        AutoRevert<bool> autoRevert(&originallyFalse, true);

C++ isn't smart enough to imply the template value?  I thought it could figure it out from the constructor.

[David Levin]

I missed adding this to header file to non-xcode build systems. (I'll fix that before landing.)

[Anders Carlsson]

Comment on attachment 117063 [details]
Patch

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

> Source/JavaScriptCore/wtf/AutoRevert.h:46
> +    AutoRevert(T* scopedVariable, T newValue)

Is there a reason why scopedVariable isn't a reference?

>> Tools/TestWebKitAPI/Tests/WTF/AutoRevert.cpp:36
>> +        AutoRevert<bool> autoRevert(&originallyFalse, true);
> 
> C++ isn't smart enough to imply the template value?  I thought it could figure it out from the constructor.

Nope, that's not how class templates work - C++ first parses the variable declaration and then the constructor arguments.

[David Levin]

(In reply to comment #4)
> (From update of attachment 117063 [details])
> View in context: https://bugs.webkit.org/attachment.cgi?id=117063&action=review
> 
> > Source/JavaScriptCore/wtf/AutoRevert.h:46
> > +    AutoRevert(T* scopedVariable, T newValue)
> 
> Is there a reason why scopedVariable isn't a reference?

I'll change that. It will still be stored internally in the calss as a pointer to allow for clearing it out when revertNow is called.

[Anders Carlsson]

Comment on attachment 117063 [details]
Patch

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

>>> Source/JavaScriptCore/wtf/AutoRevert.h:46
>>> +    AutoRevert(T* scopedVariable, T newValue)
>> 
>> Is there a reason why scopedVariable isn't a reference?
> 
> I'll change that. It will still be stored internally in the calss as a pointer to allow for clearing it out when revertNow is called.

Great!

> Source/JavaScriptCore/wtf/AutoRevert.h:64
> +        *m_scopedVariable = m_originalValue;
> +        m_scopedVariable = 0;

You could also assert that m_scopedVariable is not null here.

[David Levin]

Created attachment 117105 [details]
Patch

[David Levin]

Comment on attachment 117105 [details]
Patch

Ok, speak up if anyone objects. This is slightly different from before. (Different names, removed a method, changed the variable from being a T* to a T&.)

Since I've received and incorporated (valuable) input from Adam, Alexey, Anders, and Dmitry Lomov, I think this bikeshed has been well painted :).

I'll cq+ it after it passes enough ews bots.

[David Levin]

Comment on attachment 117105 [details]
Patch

Committed as http://trac.webkit.org/changeset/101446.

[Darin Adler]

I think TemporaryChange would be a good name; classes normally want nouns as their names, not verbs.

[David Levin]

(In reply to comment #10)
> I think TemporaryChange would be a good name; classes normally want nouns as their names, not verbs.

Makes sense. I'll do a follow up patch... :)