Bug 196465 – Add SaveRestoreScope, which can be used to save/restore state without direct access to member variables

• Status: NEW
• Component: New Bugs

[Simon Fraser (smfr)]

Add SaveRestoreScope, which can be used to save/restore state without direct access to member variables

[Simon Fraser (smfr)]

Created attachment 366426 [details]
Patch

[Simon Fraser (smfr)]

I considered pointers to member functions, but it gets pretty ugly, and you'd end up with template instantiation per class/getter/setter combination.

[EWS Watchlist]

Attachment 366426 [details] did not pass style-queue:


ERROR: Source/WTF/wtf/SetForScope.h:73:  Extra space before ( in function call  [whitespace/parens] [4]
ERROR: Source/WTF/wtf/SetForScope.h:79:  Extra space before ( in function call  [whitespace/parens] [4]
ERROR: Source/WTF/wtf/SetForScope.h:93:  Extra space before ( in function call  [whitespace/parens] [4]
ERROR: Tools/TestWebKitAPI/Tests/WTF/SetForScope.cpp:62:  More than one command on the same line  [whitespace/newline] [4]
ERROR: Tools/TestWebKitAPI/Tests/WTF/SetForScope.cpp:70:  More than one command on the same line  [whitespace/newline] [4]
ERROR: Tools/TestWebKitAPI/Tests/WTF/SetForScope.cpp:83:  More than one command on the same line  [whitespace/newline] [4]
ERROR: Tools/TestWebKitAPI/Tests/WTF/SetForScope.cpp:86:  More than one command on the same line  [whitespace/newline] [4]
Total errors found: 7 in 4 files


If any of these errors are false positives, please file a bug against check-webkit-style.

[Alex Christensen]

Comment on attachment 366426 [details]
Patch

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

Could you use this in WebKit in a place where SetForScope is insufficient?  Otherwise we have unused code in WTF, which is undesirable.

> Source/WTF/wtf/SetForScope.h:93
> +    WTF::Function<void (T)> m_setterFunction;

No space after void.
WTF:: is not needed.

[Simon Fraser (smfr)]

Created attachment 366432 [details]
Patch

[EWS Watchlist]

Attachment 366432 [details] did not pass style-queue:


ERROR: Source/WebKitLegacy/mac/WebView/WebHTMLView.mm:6888:  More than one command on the same line  [whitespace/newline] [4]
ERROR: Tools/TestWebKitAPI/Tests/WTF/SetForScope.cpp:62:  More than one command on the same line  [whitespace/newline] [4]
ERROR: Tools/TestWebKitAPI/Tests/WTF/SetForScope.cpp:70:  More than one command on the same line  [whitespace/newline] [4]
ERROR: Tools/TestWebKitAPI/Tests/WTF/SetForScope.cpp:83:  More than one command on the same line  [whitespace/newline] [4]
ERROR: Tools/TestWebKitAPI/Tests/WTF/SetForScope.cpp:86:  More than one command on the same line  [whitespace/newline] [4]
ERROR: Source/WTF/wtf/SetForScope.h:93:  Extra space before ( in function call  [whitespace/parens] [4]
ERROR: Source/WebCore/page/FrameView.cpp:4260:  More than one command on the same line  [whitespace/newline] [4]
Total errors found: 7 in 8 files


If any of these errors are false positives, please file a bug against check-webkit-style.

[Saam Barati]

Comment on attachment 366432 [details]
Patch

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

> Source/WTF/wtf/SetForScope.h:71
> +class SaveRestoreScope {

Can we just make SetForScope also have a constructor that handles this? I prefer that name more than this

> Source/WTF/wtf/SetForScope.h:73
> +    SaveRestoreScope(Function<T()>&& getterFunction, Function<void(T)>&& setterFunction)

Why not “const Function<T()>&” for the type of getter function?

> Source/WTF/wtf/SetForScope.h:79
> +    SaveRestoreScope(Function<T()>&& getterFunction, Function<void(T)>&& setterFunction, T newValue)

Ditto

[Saam Barati]

Comment on attachment 366432 [details]
Patch

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

> Source/WTF/wtf/SetForScope.h:88
> +        m_setterFunction(m_originalValue);

WTFMove? Or perhaps forwarding semantics?

[Simon Fraser (smfr)]

(In reply to Saam Barati from comment #7)
> Comment on attachment 366432 [details]
> Patch
> 
> View in context:
> https://bugs.webkit.org/attachment.cgi?id=366432&action=review
> 
> > Source/WTF/wtf/SetForScope.h:71
> > +class SaveRestoreScope {
> 
> Can we just make SetForScope also have a constructor that handles this? I
> prefer that name more than this

That would bloat SetForScope with WTF::Function, but maybe that's OK. This is also a little more loosey-goosey than SetForScope because those two functors could do anything.


> > Source/WTF/wtf/SetForScope.h:73
> > +    SaveRestoreScope(Function<T()>&& getterFunction, Function<void(T)>&& setterFunction)
> 
> Why not “const Function<T()>&” for the type of getter function?

I never know when to pass Functions by reference or RVF.

> > Source/WTF/wtf/SetForScope.h:79
> > +    SaveRestoreScope(Function<T()>&& getterFunction, Function<void(T)>&& setterFunction, T newValue)

> WTFMove? Or perhaps forwarding semantics?

Sure, I guess? Does that add any overhead when used with small POD types?