Bug 184169 – Introduce WTF_LAZY_INSTANTIATE

• Status: RESOLVED
• Resolution: FIXED
• Component: Web Template Framework

[JF Bastien]

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

[Radar WebKit Bug Importer]

<rdar://problem/39023385>

[JF Bastien]

Created attachment 336831 [details]
patch

[EWS Watchlist]

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.

[Mark Lam]

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.

[Mark Lam]

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 "...".

[JF Bastien]

Created attachment 336866 [details]
patch for landing, rebased onto my other patch which touched WTFString.h

[WebKit Commit Bot]

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>

[WebKit Commit Bot]

All reviewed patches have been landed.  Closing bug.

[Filip Pizlo]

Can we roll this out?

[Alex Christensen]

(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?

[JF Bastien]

(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.

[Konstantin Tokarev]

IMO name "...INSTANTIATE" implies (template) instantiation, while what actually happens here is delayed type name lookup

[JF Bastien]

(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.

[Konstantin Tokarev]

BTW, did you consider *Inlines.h way?

[JF Bastien]

(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.

[Filip Pizlo]

(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.

[JF Bastien]

(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.

[Filip Pizlo]

(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.

[Alex Christensen]

I guess we should roll this out then and put it back in with adoption if we are going to use it.