Closed Bug 989460 Opened 10 years ago Closed 10 years ago

Add MacroArgs.h for macros related to implementing variadic macros with optional args

Categories

(Core :: MFBT, defect)

defect
Not set
normal

Tracking

()

RESOLVED FIXED
mozilla31

People

(Reporter: poiru, Assigned: poiru)

References

Details

Attachments

(3 files, 6 obsolete files)

This will be used e.g. to create a variadic NS_IMPL_ISUPPORTS macro.
Blocks: 989465
If you can make it work, all power to you.  But MSVC has some variadic macro expansion bugs, that -- as far as glandium could tell when he tried to consolidate -- makes it impossible to do this.  I'll await try results.  :-)
This is similar to what i did in bug 900903, but that falls short on complex things.
(In reply to Jeff Walden [:Waldo] (remove +bmo to email) from comment #2)
> If you can make it work, all power to you.  But MSVC has some variadic macro
> expansion bugs, that -- as far as glandium could tell when he tried to
> consolidate -- makes it impossible to do this.  I'll await try results.  :-)

I made it work, see try push: https://tbpl.mozilla.org/?tree=Try&rev=fbf2c396891f
Attachment #8398695 - Attachment is obsolete: true
Attachment #8398695 - Flags: review?(jwalden+bmo)
Attachment #8399073 - Flags: review?(jwalden+bmo)
Blocks: 900903
Blocks: 900908
Attachment #8399073 - Attachment is obsolete: true
Attachment #8399073 - Flags: review?(jwalden+bmo)
Attachment #8407751 - Flags: review?(jwalden+bmo)
Attached patch Part 2: Add MOZ_FOR_EACH macro (obsolete) — Splinter Review
Attachment #8407753 - Flags: review?(jwalden+bmo)
Comment on attachment 8407751 [details] [diff] [review]
Part 1: Add MacroArgs.h for macros related to implementing variadic macros

Review of attachment 8407751 [details] [diff] [review]:
-----------------------------------------------------------------

So, I had most of these comments written up by the time you posted the newer version yesterday.  I imagine some/many still apply, so putting it out there even tho it's obsoleted.  I'll try to skim an interdiff of the two patches and figure out what changes are/aren't important to examine, or something, still.

::: mfbt/Assertions.h
@@ +294,5 @@
>         MOZ_REALLY_CRASH(); \
>       } \
>     } while (0)
> +
> +#define MOZ__RELEASE_ASSERT_GLUE(a, b) a b

Names containing two consecutive underscores are reserved in C++, so get rid of one of the underscores here.

::: mfbt/MacroArgs.h
@@ +10,5 @@
> +/*
> + * MOZ_COUNT_ARGS_PREFIXED(aPrefix, ...) counts the number of variadic
> + * arguments and prefixes it with |aPrefix|. For example:
> + *
> + *   MOZ_COUNT_ARGS_PREFIXED(A, B, foo, 42, bar) expands to A3B

Erm.  Did you mean A3?  A3B is mightily confusing!

@@ +16,5 @@
> + * Up to 50 variadic arguments may be passed. If the limit is exceeded, this
> + * macro will actually expand to the 51st variadic argument. If a suitable
> + * prefix is used and if the result of the expansion is e.g. the name of
> + * another macro to invoke, exceeding the limit will most likely result in a
> + * compile-time error.

I'm not hugely worried about people passing too many arguments, not when the numbers are this high.  I'd trim this paragraph like so:

"""
Up to 50 variadic arguments may be passed.  If you pass additional arguments, 1) your code should almost certainly be refactored, and 2) your code won't work.
"""

Oversharing about what'll happen just makes people think they can do it with impunity.  :-)

@@ +19,5 @@
> + * another macro to invoke, exceeding the limit will most likely result in a
> + * compile-time error.
> + *
> + * When given no variadic arguments, this still expands to 1. To reject these
> + * cases, use the MOZ_ASSERT_VALID_ARG_COUNT macro.

You mean MOZ_COUNT_ARGS_PREFIXED()?  C++11 says "there shall be more arguments in the invocation than there are parameters in the macro definition (excluding the ...)."  So this is wrong.  To the extent compilers do anything here, they're either wrong or extending the language, not sure which.  Remove this paragraph.

@@ +22,5 @@
> + * When given no variadic arguments, this still expands to 1. To reject these
> + * cases, use the MOZ_ASSERT_VALID_ARG_COUNT macro.
> + *
> + * This very carefully tiptoes around a MSVC bug where it improperly expands
> + * __VA_ARGS__ as a single token in argument lists. For details, see:

Maybe

"""
Passing (__VA_ARGS__, <rest of arguments>) rather than simply calling MOZ_MACROARGS_COUNT_ARGS_HELPER2(__VA_ARGS__, <rest of arguments>) very carefully tiptoes around an MSVC bug where it improperly expands __VA_ARGS__ as a single token in argument lists. For details, see:
"""

to be slightly more precise about what the tiptoeing is.

@@ +28,5 @@
> + *   http://connect.microsoft.com/VisualStudio/feedback/details/380090/variadic-macro-replacement
> + *   http://cplusplus.co.il/2010/07/17/variadic-macro-to-count-number-of-arguments/#comment-644
> + */
> +#define MOZ_COUNT_ARGS_PREFIXED(aPrefix, ...) \
> +  MOZ__MACROARGS_COUNT_ARGS_HELPER((__VA_ARGS__, \

No double-underscores in names.

@@ +41,5 @@
> +    aPrefix##10, aPrefix##9,  aPrefix##8,  aPrefix##7,  aPrefix##6,  \
> +    aPrefix##5,  aPrefix##4,  aPrefix##3,  aPrefix##2,  aPrefix##1, aPrefix##0))
> +
> +#define MOZ__MACROARGS_COUNT_ARGS_HELPER(aArgs) \
> +  MOZ__MACROARGS_COUNT_ARGS_HELPER2 aArgs

And again.

@@ +64,5 @@
> + * When given no variadic arguments, this still expands to 1. To reject these
> + * cases, use the MOZ_ASSERT_VALID_ARG_COUNT macro.
> + */
> +#define MOZ_COUNT_ARGS_AS_EXPR(...) \
> +  (MOZ_COUNT_ARGS_PREFIXED(/* empty */, __VA_ARGS__) / 1)

Hmm.  In C++98 it was undefined behavior to pass a macro argument expanding to no tokens.  (Comments get removed before this phase.)  So this is only valid as C++11.  I think most compilers would emit a warning in C++98 code that used this, then do the "expected" thing.  We should be on the lookout for compiler warnings resulting from this.  (Although dholbert tells me he hasn't seen this warning in awhile, so we're probably safe.)

@@ +75,5 @@
> + * This macro employs a few dirty tricks to function. First, in order to
> + * detect the no arguments case, |(__VA_ARGS__)| is stringified and compared
> + * to what it becomes in the in the absence of arguments. Then, to detect too
> + * too many arguments, MOZ_COUNT_ARGS_PREFIXED is used with a prefix of 0.0.
> + * For any valid numbero arguments, this will expand to e.g. 0.04. If the

number of?

@@ +76,5 @@
> + * detect the no arguments case, |(__VA_ARGS__)| is stringified and compared
> + * to what it becomes in the in the absence of arguments. Then, to detect too
> + * too many arguments, MOZ_COUNT_ARGS_PREFIXED is used with a prefix of 0.0.
> + * For any valid numbero arguments, this will expand to e.g. 0.04. If the
> + * maximum argument limit it exceeded, the MOZ_COUNT_ARGS* macros

is exceeded?

@@ +82,5 @@
> + * a number that is both >= 1 and < 0.1. Consequently, if the first argument
> + * over the limit is a number, at least one of the static_asserts will fail.
> + * If it is anything else, a compile-time error of some kind will occur.
> + */
> +#define MOZ__MACROARGS_STRINGIFY(x) #x

No double-underscores.

@@ +84,5 @@
> + * If it is anything else, a compile-time error of some kind will occur.
> + */
> +#define MOZ__MACROARGS_STRINGIFY(x) #x
> +#define MOZ_ASSERT_VALID_ARG_COUNT(...) \
> +  static_assert( \

This should be contained within a do {} while (false) for the greatest statement-like usability.

@@ +114,5 @@
> + */
> +#define MOZ_COMBINE_TOKENS(a, b) \
> +  MOZ__MACROARGS_COMBINE_TOKENS_HELPER(a, b)
> +
> +#define MOZ__MACROARGS_COMBINE_TOKENS_HELPER(a, b) \

No double-underscores.

@@ +117,5 @@
> +
> +#define MOZ__MACROARGS_COMBINE_TOKENS_HELPER(a, b) \
> +  MOZ__MACROARGS_COMBINE_TOKENS_HELPER2(a, b)
> +
> +#define MOZ__MACROARGS_COMBINE_TOKENS_HELPER2(a, b) \

No double-underscores.

::: mfbt/TypedEnum.h
@@ +290,5 @@
>     */
>  #  define MOZ_TEMPLATE_ENUM_CLASS_ENUM_TYPE(Name) typename Name::Enum
>  #endif
>  
> +#  define MOZ__BEGIN_NESTED_ENUM_CLASS_GLUE(a, b) a b

Double-underscores.
Attachment #8407751 - Flags: review?(jwalden+bmo)
Or no, somehow I *did* review the correct patch here.  Not sure what's up, exactly, with my memory.  Guess I'm getting old.  ;-)
Comment on attachment 8407753 [details] [diff] [review]
Part 2: Add MOZ_FOR_EACH macro

Review of attachment 8407753 [details] [diff] [review]:
-----------------------------------------------------------------

::: mfbt/MacroForEach.h
@@ +3,5 @@
> +/* This Source Code Form is subject to the terms of the Mozilla Public
> + * License, v. 2.0. If a copy of the MPL was not distributed with this
> + * file, You can obtain one at http://mozilla.org/MPL/2.0/. */
> +
> +#ifndef mozilla_MacroForEach_h

Needs an overview comment like all the other mfbt headers have.  Something like this maybe (suggest better ways to say this if you have 'em, it's a rather generic thing to describe concisely):

/* A higher-order macro for iteratively calling another macro with fixed leading arguments, plus a trailing element picked from a second list of arguments. */

@@ +34,5 @@
> + * |aArgs| must not be empty. Use MOZ_ASSERT_VALID_ARG_COUNT to verify this if
> + * needed.
> + */
> +#define MOZ__FOR_EACH_GLUE(a, b) a b
> +#define MOZ__FOR_EACH_EXPAND(...) __VA_ARGS__

Double-underscores.

@@ +41,5 @@
> +    MOZ_COUNT_ARGS_PREFIXED(MOZ__FOR_, MOZ__FOR_EACH_EXPAND aArgs), \
> +    (aMacro, aFixedArgs, aArgs))
> +
> +#define MOZ__FOR_GLUE(a, b) a b
> +#define MOZ__FOR(aMacro, aFixedArgs, aArgs) \

And more.

@@ +45,5 @@
> +#define MOZ__FOR(aMacro, aFixedArgs, aArgs) \
> +  MOZ__FOR_GLUE(aMacro, (MOZ__FOR_EACH_EXPAND aFixedArgs MOZ_ARG_1 aArgs))
> +
> +#define MOZ__FOR_1(m, fa, a) MOZ__FOR(m, fa, a)
> +#define MOZ__FOR_2(m, fa, a) MOZ__FOR_1(m, fa, (MOZ_ARGS_AFTER_1 a))MOZ__FOR(m, fa, a)

I'd prefer a space between the )) and the MOZ__FOR there, for readability.  Is there a particular reason you don't have one?
Attachment #8407753 - Flags: review?(jwalden+bmo) → review+
(In reply to Jeff Walden [:Waldo] (remove +bmo to email) from comment #8)
> @@ +16,5 @@
> > + * Up to 50 variadic arguments may be passed. If the limit is exceeded, this
> > + * macro will actually expand to the 51st variadic argument. If a suitable
> > + * prefix is used and if the result of the expansion is e.g. the name of
> > + * another macro to invoke, exceeding the limit will most likely result in a
> > + * compile-time error.
> > + *
> > + * When given no variadic arguments, this still expands to 1. To reject these
> > + * cases, use the MOZ_ASSERT_VALID_ARG_COUNT macro.
> 
> You mean MOZ_COUNT_ARGS_PREFIXED()?  C++11 says "there shall be more
> arguments in the invocation than there are parameters in the macro
> definition (excluding the ...)."  So this is wrong.  To the extent compilers
> do anything here, they're either wrong or extending the language, not sure
> which.  Remove this paragraph.

I actually did mean MOZ_COUNT_ARGS_PREFIXED. I clarified the comment -- let me know if it makes sense now.

> @@ +64,5 @@
> > + * When given no variadic arguments, this still expands to 1. To reject these
> > + * cases, use the MOZ_ASSERT_VALID_ARG_COUNT macro.
> > + */
> > +#define MOZ_COUNT_ARGS_AS_EXPR(...) \
> > +  (MOZ_COUNT_ARGS_PREFIXED(/* empty */, __VA_ARGS__) / 1)
> 
> Hmm.  In C++98 it was undefined behavior to pass a macro argument expanding
> to no tokens.  (Comments get removed before this phase.)  So this is only
> valid as C++11.  I think most compilers would emit a warning in C++98 code
> that used this, then do the "expected" thing.  We should be on the lookout
> for compiler warnings resulting from this.  (Although dholbert tells me he
> hasn't seen this warning in awhile, so we're probably safe.)

I removed MOZ_COUNT_ARGS_AS_EXPR entirely and went with slightly a different approach. However, the tests still MOZ_COUNT_ARGS_PREFIXED with an empty prefix.

> @@ +84,5 @@
> > + * If it is anything else, a compile-time error of some kind will occur.
> > + */
> > +#define MOZ__MACROARGS_STRINGIFY(x) #x
> > +#define MOZ_ASSERT_VALID_ARG_COUNT(...) \
> > +  static_assert( \
> 
> This should be contained within a do {} while (false) for the greatest
> statement-like usability.

That would constraint the macro to places where a do-block is allowed. To achieve the same effect with the constraint, I removed the semi-colon of the second static_assert.
Attachment #8407751 - Attachment is obsolete: true
Attachment #8408960 - Flags: review?(jwalden+bmo)
Comment on attachment 8408960 [details] [diff] [review]
Part 1: Add MacroArgs.h for macros related to implementing variadic macros

Review of attachment 8408960 [details] [diff] [review]:
-----------------------------------------------------------------

::: mfbt/MacroArgs.h
@@ +8,5 @@
> +#define mozilla_MacroArgs_h
> +
> +/*
> + * Implements various macros meant to ease the use of variadic macros.
> + */

This should go outside the ifdefs, I believe, for consistency with all the other headers.

@@ +14,5 @@
> +/*
> + * MOZ_COUNT_ARGS_PREFIXED(aPrefix, ...) counts the number of variadic
> + * arguments and prefixes it with |aPrefix|. For example:
> + *
> + *   MOZ_COUNT_ARGS_PREFIXED(, foo, 42, bar) expands to 3

Maybe change this to |MOZ_COUNT_ARGS_PREFIXED(, foo, 42) expands to 2| to give an example of one more arity being used.

@@ +18,5 @@
> + *   MOZ_COUNT_ARGS_PREFIXED(, foo, 42, bar) expands to 3
> + *   MOZ_COUNT_ARGS_PREFIXED(A, foo, 42, bar) expands to A3
> + *
> + * The number of variadic arguments must be between 1 and 50 inclusive. If you
> + * don't pass any variadic arguments, this will expand to 1 (instead of 0).

(In reply to Birunthan Mohanathas [:poiru] from comment #11)
> > You mean MOZ_COUNT_ARGS_PREFIXED()?  C++11 says "there shall be more
> > arguments in the invocation than there are parameters in the macro
> > definition (excluding the ...)."  So this is wrong.  To the extent compilers
> > do anything here, they're either wrong or extending the language, not sure
> > which.  Remove this paragraph.
> 
> I actually did mean MOZ_COUNT_ARGS_PREFIXED. I clarified the comment -- let
> me know if it makes sense now.

No, what I meant is that it's not legal C++ for someone to do MOZ_COUNT_ARGS_PREFIXED() or MOZ_COUNT_ARGS_PREFIXED(prefix) at all.  You "shall" pass in more arguments than there are non-... parameters in the macro definition -- that is, more than 1.  So what you *should* be saying, is that it's not legal to pass in zero variadic arguments.  You have to pass in at least one, and if you pass in zero, your code is non-standard.  So say something like

"""
You must pass in between 1 and 50 (inclusive) variadic arguments, past aPrefix.  It is not legal to do

  MOZ_COUNT_ARGS_PREFIXED(prefix)

(that is, pass in 0 variadic arguments).  If you do it, you won't get any sort of desirable behavior.
"""

On the topic of names, I haven't really been thinking about this name.  But might MOZ_PASTE_PREFIX_AND_ARGCOUNT be a better name?  The result is the prefix pasted to the argument count, so this name seems to clearly indicate the ultimate expansion, directly, in the order it appears in the expansion.

@@ +64,5 @@
> + * same variadic arguments.
> + *
> + * This macro employs a few dirty tricks to function. To detect the zero
> + * argument case, |(__VA_ARGS__)| is stringified, sizeof-ed, and compared to
> + * what it should be in the absense of arguments.

absence

@@ +76,5 @@
> + * number, a compile-time error will still occur because the exceeding
> + * argument is compared to an int and a double.
> + */
> +#define MOZ_ASSERT_VALID_ARG_COUNT_STRINGIFY_HELPER(x) #x
> +#define MOZ_ASSERT_VALID_ARG_COUNT(...) \

Let's add STATIC into this name, so MOZ_STATIC_ASSERT_....

@@ +84,5 @@
> +    "Too few arguments for MOZ_ASSERT_VALID_ARG_COUNT"); \
> +  static_assert( \
> +    MOZ_COUNT_ARGS_PREFIXED(/* empty */, __VA_ARGS__) >= 1 && \
> +      MOZ_COUNT_ARGS_PREFIXED(0.0, __VA_ARGS__) < 0.1, \
> +    "Too many arguments for MOZ_ASSERT_VALID_ARG_COUNT") /* No semicolon */

(In reply to Birunthan Mohanathas [:poiru] from comment #11)
> That would constraint the macro to places where a do-block is allowed. To
> achieve the same effect with the constraint, I removed the semi-colon of the
> second static_assert.

Hmm.  The reason I asked about a do-while was because this won't work as a single statement of its own.  The point about its being usable in all static_assert contexts, tho, makes sense.  So just combine the two static_asserts into one to avoid this, and make the error message something like "MOZ_ASSERT_VALID_ARG_COUNT requires 1 to 51 arguments" or so.

::: mfbt/tests/TestMacroArgs.cpp
@@ +15,5 @@
> +
> +static_assert(MOZ_COUNT_ARGS_PREFIXED(, MOZ_ARGS_AFTER_1(a, b, c)) == 2,
> +              "ARGS_AFTER_1(a, b, c) should expand to 'b, c'");
> +static_assert(MOZ_ARGS_AFTER_2(a, b, 3) == 3,
> +              "ARGS_AFTER_2(a, b, 3) should expand to '3'");

MOZ_ARGS_AFTER in the reason, same above it.
Attachment #8408960 - Flags: review?(jwalden+bmo) → review+
Attachment #8407753 - Attachment is obsolete: true
Attachment #8411967 - Flags: review+
Just a nit:
>MOZ_STATIC_ASSERT_VALID_ARG_COUNT(,,,,,,,,,,,,,,,,,,,,,,,,,,,,,,,,,,,,,,,,,,,,,,,,,,true!=1)
asserts
>sizeof("(,,,,,,,,,,,,,,,,,,,,,,,,,,,,,,,,,,,,,,,,,,,,,,,,,,true!=1)") != sizeof("()") && true!=1>10 && true!=1<0.1
which to my untrained eye looks as if it should evaluate to true.
https://hg.mozilla.org/mozilla-central/rev/c7399918527a
https://hg.mozilla.org/mozilla-central/rev/910de4b74638
Status: ASSIGNED → RESOLVED
Closed: 10 years ago
Resolution: --- → FIXED
Target Milestone: --- → mozilla31
(In reply to neil@parkwaycc.co.uk from comment #17)
> Just a nit:
> >MOZ_STATIC_ASSERT_VALID_ARG_COUNT(,,,,,,,,,,,,,,,,,,,,,,,,,,,,,,,,,,,,,,,,,,,,,,,,,,true!=1)
> asserts
> >sizeof("(,,,,,,,,,,,,,,,,,,,,,,,,,,,,,,,,,,,,,,,,,,,,,,,,,,true!=1)") != sizeof("()") && true!=1>10 && true!=1<0.1
> which to my untrained eye looks as if it should evaluate to true.

Fixed, thanks!
Attachment #8412665 - Flags: review+
Keywords: checkin-needed
Fixed bug number.
Attachment #8412665 - Attachment is obsolete: true
Attachment #8412712 - Flags: review+
You need to log in before you can comment on or make changes to this bug.