Bug 164761 - [JSC] WTF::TemporaryChange with WTF::SetForScope
Summary: [JSC] WTF::TemporaryChange with WTF::SetForScope
Status: RESOLVED FIXED
Alias: None
Product: WebKit
Classification: Unclassified
Component: JavaScriptCore (show other bugs)
Version: WebKit Nightly Build
Hardware: Unspecified Unspecified
: P2 Normal
Assignee: Yusuke Suzuki
URL:
Keywords:
Depends on:
Blocks:
 
Reported: 2016-11-14 20:12 PST by Yusuke Suzuki
Modified: 2016-12-23 11:17 PST (History)
2 users (show)

See Also:


Attachments
Patch (97.88 KB, patch)
2016-11-16 23:20 PST, Yusuke Suzuki
no flags Details | Formatted Diff | Diff
Patch (97.87 KB, patch)
2016-11-16 23:45 PST, Yusuke Suzuki
no flags Details | Formatted Diff | Diff
Patch (97.97 KB, patch)
2016-11-17 00:17 PST, Yusuke Suzuki
sbarati: review+
Details | Formatted Diff | Diff

Note You need to log in before you can comment on or make changes to this bug.
Description Yusuke Suzuki 2016-11-14 20:12:10 PST
It's completely the same.
Comment 1 Saam Barati 2016-11-14 22:49:59 PST
(In reply to comment #0)
> It's completely the same.

Lol.

FWIW, I prefer the name "SetForScope" over "TemporaryChange"
Comment 2 Yusuke Suzuki 2016-11-16 23:20:29 PST
Created attachment 295033 [details]
Patch
Comment 3 Yusuke Suzuki 2016-11-16 23:45:39 PST
Created attachment 295035 [details]
Patch
Comment 4 Yusuke Suzuki 2016-11-17 00:17:15 PST
Created attachment 295038 [details]
Patch
Comment 5 Saam Barati 2016-11-17 00:23:04 PST
Comment on attachment 295038 [details]
Patch

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

r=me

> Source/WTF/wtf/SetForScope.h:51
> +    SetForScope(T& scopedVariable, T newValue)

Might be nice to use forward semantics here.

> Source/WTF/wtf/SetForScope.h:59
> +        m_scopedVariable = m_originalValue;

Maybe move semantics?
Comment 6 Yusuke Suzuki 2016-11-17 00:43:38 PST
Comment on attachment 295038 [details]
Patch

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

Thanks!

>> Source/WTF/wtf/SetForScope.h:51
>> +    SetForScope(T& scopedVariable, T newValue)
> 
> Might be nice to use forward semantics here.

Yeah, fixed. Using argument deduction and std::forward here.

>> Source/WTF/wtf/SetForScope.h:59
>> +        m_scopedVariable = m_originalValue;
> 
> Maybe move semantics?

Yeah, we can use `WTFMove()`.
Comment 7 Yusuke Suzuki 2016-11-17 00:48:44 PST
Committed r208841: <http://trac.webkit.org/changeset/208841>
Comment 8 Simon Fraser (smfr) 2016-12-22 16:34:54 PST
I think this rename should have been communicated with webkit-dev.
Comment 9 Saam Barati 2016-12-22 18:20:48 PST
(In reply to comment #8)
> I think this rename should have been communicated with webkit-dev.

Fair enough. We can always name it back.
Comment 10 Saam Barati 2016-12-22 18:23:54 PST
(In reply to comment #9)
> (In reply to comment #8)
> > I think this rename should have been communicated with webkit-dev.
> 
> Fair enough. We can always name it back.

I'll send out an email to webkit-dev now.
Comment 11 Simon Fraser (smfr) 2016-12-23 11:17:07 PST
Would have also been nice to have some discussion about naming. This comes through as picking the JSC name by fiat.