Bug 184169 - Introduce WTF_LAZY_INSTANTIATE
Summary: Introduce WTF_LAZY_INSTANTIATE
Status: RESOLVED FIXED
Alias: None
Product: WebKit
Classification: Unclassified
Component: Web Template Framework (show other bugs)
Version: WebKit Nightly Build
Hardware: Unspecified Unspecified
: P2 Normal
Assignee: JF Bastien
URL:
Keywords: InRadar
Depends on:
Blocks: 184164
  Show dependency treegraph
 
Reported: 2018-03-29 18:48 PDT by JF Bastien
Modified: 2018-03-30 13:34 PDT (History)
14 users (show)

See Also:


Attachments
patch (4.74 KB, patch)
2018-03-29 18:49 PDT, JF Bastien
mark.lam: review+
Details | Formatted Diff | Diff
patch for landing, rebased onto my other patch which touched WTFString.h (5.06 KB, patch)
2018-03-30 09:59 PDT, JF Bastien
no flags Details | Formatted Diff | Diff

Note You need to log in before you can comment on or make changes to this bug.
Description JF Bastien 2018-03-29 18:48:06 PDT
As part of #184164 I'm adding some forwarding methods to WTF::String. These need to forward RetainPtr and Cstring, and usually that would require adding RetainPtr.h and CString.h to WTFString.h which isn't really something we should do.

Introduce WTF_LAZY_INSTANTIATE to forward-declare functions which just pass parameters to another function, and return whatever that other function returned, without having to include the return's / parameters' type header.

Try it out here: godbolt.org/g/oV8G5Q
Comment 1 Radar WebKit Bug Importer 2018-03-29 18:48:29 PDT
<rdar://problem/39023385>
Comment 2 JF Bastien 2018-03-29 18:49:56 PDT
Created attachment 336831 [details]
patch
Comment 3 EWS Watchlist 2018-03-29 18:51:58 PDT
Attachment 336831 [details] did not pass style-queue:


ERROR: Source/WTF/wtf/Forward.h:147:  Missing space after ,  [whitespace/comma] [3]
Total errors found: 1 in 2 files


If any of these errors are false positives, please file a bug against check-webkit-style.
Comment 4 Mark Lam 2018-03-29 21:44:32 PDT
Comment on attachment 336831 [details]
patch

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

r=me with suggestions.

> Source/WTF/ChangeLog:10
> +        WTF::String. These need to forward RetainPtr and Cstring, and

typo: /Cstring/CString/.

> Source/WTF/ChangeLog:11
> +        usually that would require adding RetainPtr.h and CString.h to

nit: I suggest saying "#include'ing" instead of "adding".

> Source/WTF/wtf/Forward.h:147
> +#define WTF_LAZY_CALL_EACH(F,...) \

I suggest renaming WTF_LAZY_CALL_EACH to WTF_LAZY_FOR_EACH_TERM_IN_VA_ARGS.

> Source/WTF/wtf/Forward.h:149
> +#define WTF_LAZY_LAST(_1, ...) _1

You're actually picking off the first arg, not the last.  So, I suggest renaming this to WTF_LAZY_FIRST_TERM_IN_VA_ARGS.

> Source/WTF/wtf/Forward.h:150
> +#define WTF_LAZY_MORE(_1, ...) (__VA_ARGS__)

I suggest renaming WTF_LAZY_MORE to WTF_LAZY_REST_OF_TERMS_IN_VA_ARGS.

> Source/WTF/wtf/Forward.h:158
> +#define WTF_LAZY_CALL_EACH_1(F, A) F(WTF_LAZY_LAST A) WTF_LAZY_CALL_EACH_0(F, WTF_LAZY_MORE A)
> +#define WTF_LAZY_CALL_EACH_2(F, A) F(WTF_LAZY_LAST A) WTF_LAZY_CALL_EACH_1(F, WTF_LAZY_MORE A)
> +#define WTF_LAZY_CALL_EACH_3(F, A) F(WTF_LAZY_LAST A) WTF_LAZY_CALL_EACH_2(F, WTF_LAZY_MORE A)
> +#define WTF_LAZY_CALL_EACH_4(F, A) F(WTF_LAZY_LAST A) WTF_LAZY_CALL_EACH_3(F, WTF_LAZY_MORE A)
> +#define WTF_LAZY_CALL_EACH_5(F, A) F(WTF_LAZY_LAST A) WTF_LAZY_CALL_EACH_4(F, WTF_LAZY_MORE A)
> +#define WTF_LAZY_CALL_EACH_6(F, A) F(WTF_LAZY_LAST A) WTF_LAZY_CALL_EACH_5(F, WTF_LAZY_MORE A)
> +#define WTF_LAZY_CALL_EACH_7(F, A) F(WTF_LAZY_LAST A) WTF_LAZY_CALL_EACH_6(F, WTF_LAZY_MORE A)

I suggest renaming A to ArgsInParens.  Took me a while to catch on that WTF_LAZY_CALL_EACH passes in (__VA_ARGS__) as A, and hence WTF_LAZY_LAST A expands to WTF_LAZY_LAST(__VA_ARGS__).  It made sense thereafter, but was not so obvious in the beginning.

I would also suggest renaming WTF_LAZY_CALL_EACH_7 to WTF_LAZY_PROCESS_7 (and similarly for 6 to 0).

> Source/WTF/wtf/Forward.h:159
> +#define WTF_LAZY_TYPE(ALIAS_AND_TYPE) typename ALIAS_AND_TYPE,

I suggest renaming WTF_LAZY_TYPE to WTF_LAZY_DECLARE_TYPE.

So, FOR_EACH_TERM_IN_VA_ARGS, you PROCESS a term, where you pick off the FIRST_TERM_IN_VA_ARGS from the ArgsInParens, DECLARE its TYPE, and pass on the REST_OF_TERMS_IN_VA_ARGS in the ArgsInParens to be processed by the next PROCESS macro.
Comment 5 Mark Lam 2018-03-29 21:50:25 PDT
Comment on attachment 336831 [details]
patch

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

>> Source/WTF/wtf/Forward.h:147
>> +#define WTF_LAZY_CALL_EACH(F,...) \
> 
> I suggest renaming WTF_LAZY_CALL_EACH to WTF_LAZY_FOR_EACH_TERM_IN_VA_ARGS.

Please also fix style checker complaint: add a space between "F," and "...".
Comment 6 JF Bastien 2018-03-30 09:59:35 PDT
Created attachment 336866 [details]
patch for landing, rebased onto my other patch which touched WTFString.h
Comment 7 WebKit Commit Bot 2018-03-30 10:36:24 PDT
Comment on attachment 336866 [details]
patch for landing, rebased onto my other patch which touched WTFString.h

Clearing flags on attachment: 336866

Committed r230109: <https://trac.webkit.org/changeset/230109>
Comment 8 WebKit Commit Bot 2018-03-30 10:36:26 PDT
All reviewed patches have been landed.  Closing bug.
Comment 9 Filip Pizlo 2018-03-30 10:55:52 PDT
Can we roll this out?
Comment 10 Alex Christensen 2018-03-30 10:56:45 PDT
(In reply to Filip Pizlo from comment #9)
> Can we roll this out?

I think this could be useful for other things.  Why do you want to roll it out?
Comment 11 JF Bastien 2018-03-30 10:57:39 PDT
(In reply to Alex Christensen from comment #10)
> (In reply to Filip Pizlo from comment #9)
> > Can we roll this out?
> 
> I think this could be useful for other things.  Why do you want to roll it
> out?

Right, based on Alex's comment on the bigger patch, I'm splitting things up into workable units that are easier to review.
Comment 12 Konstantin Tokarev 2018-03-30 11:15:46 PDT
IMO name "...INSTANTIATE" implies (template) instantiation, while what actually happens here is delayed type name lookup
Comment 13 JF Bastien 2018-03-30 11:20:02 PDT
(In reply to Konstantin Tokarev from comment #12)
> IMO name "...INSTANTIATE" implies (template) instantiation, while what
> actually happens here is delayed type name lookup

The macro makes the (usually non-templated function) a template, and the parameters / argument types are then delayed until the template function is called (which instantiates it). I'm not sure how to best capture that subtlety, when the net effect is that we effectively just need valid syntax in the function, and all else (handwaving here) is figured out lazily wherever the function is called.
Comment 14 Konstantin Tokarev 2018-03-30 11:27:21 PDT
BTW, did you consider *Inlines.h way?
Comment 15 JF Bastien 2018-03-30 11:40:41 PDT
(In reply to Konstantin Tokarev from comment #14)
> BTW, did you consider *Inlines.h way?

Yes, feels weird to do in WTFString.h, and this approach is way simpler IMO.
Comment 16 Filip Pizlo 2018-03-30 12:00:26 PDT
(In reply to Alex Christensen from comment #10)
> (In reply to Filip Pizlo from comment #9)
> > Can we roll this out?
> 
> I think this could be useful for other things.  Why do you want to roll it
> out?

Are those things happening now or far off in the future?

Because if they are happening far off in the future, then per WebKit policy towards dead code, we should roll it out.
Comment 17 JF Bastien 2018-03-30 12:43:40 PDT
(In reply to Filip Pizlo from comment #16)
> (In reply to Alex Christensen from comment #10)
> > (In reply to Filip Pizlo from comment #9)
> > > Can we roll this out?
> > 
> > I think this could be useful for other things.  Why do you want to roll it
> > out?
> 
> Are those things happening now or far off in the future?
> 
> Because if they are happening far off in the future, then per WebKit policy
> towards dead code, we should roll it out.

Alex requested that I split cleanup / refactor code in https://bugs.webkit.org/show_bug.cgi?id=184164 from the parts that do poisoning. This patch is one of the requested splits, and a subsequent one will use the macro. Once these cleanups / refactors are in we'll see what's left in #184164, whether it's still big or not (I expect it to be quite a bit smaller). With that we can decide whether to check it in now with the JSC usage follow-up I've got locally, or hold off.

I'm doing all of this asynchronously as I work on something else, so it may be a few days until I'm done unwinding #184164.
Comment 18 Filip Pizlo 2018-03-30 12:45:18 PDT
(In reply to JF Bastien from comment #17)
> (In reply to Filip Pizlo from comment #16)
> > (In reply to Alex Christensen from comment #10)
> > > (In reply to Filip Pizlo from comment #9)
> > > > Can we roll this out?
> > > 
> > > I think this could be useful for other things.  Why do you want to roll it
> > > out?
> > 
> > Are those things happening now or far off in the future?
> > 
> > Because if they are happening far off in the future, then per WebKit policy
> > towards dead code, we should roll it out.
> 
> Alex requested that I split cleanup / refactor code in
> https://bugs.webkit.org/show_bug.cgi?id=184164 from the parts that do
> poisoning. This patch is one of the requested splits, and a subsequent one
> will use the macro. Once these cleanups / refactors are in we'll see what's
> left in #184164, whether it's still big or not (I expect it to be quite a
> bit smaller). With that we can decide whether to check it in now with the
> JSC usage follow-up I've got locally, or hold off.
> 
> I'm doing all of this asynchronously as I work on something else, so it may
> be a few days until I'm done unwinding #184164.

We're not doing bug 184164 anymore.
Comment 19 Alex Christensen 2018-03-30 13:34:25 PDT
I guess we should roll this out then and put it back in with adoption if we are going to use it.