[webkit-reviews] review granted: [Bug 184169] Introduce WTF_LAZY_INSTANTIATE : [Attachment 336831] patch

[bugzilla-daemon at webkit.org]

Mark Lam <mark.lam at apple.com> has granted JF Bastien <jfbastien at apple.com>'s
request for review:
Bug 184169: Introduce WTF_LAZY_INSTANTIATE
https://bugs.webkit.org/show_bug.cgi?id=184169

Attachment 336831: patch

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




--- Comment #4 from Mark Lam <mark.lam at apple.com> ---
Comment on attachment 336831
  --> https://bugs.webkit.org/attachment.cgi?id=336831
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.