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)
Core
MFBT
Tracking
()
RESOLVED
FIXED
mozilla31
People
(Reporter: poiru, Assigned: poiru)
References
Details
Attachments
(3 files, 6 obsolete files)
12.86 KB,
patch
|
poiru
:
review+
|
Details | Diff | Splinter Review |
10.62 KB,
patch
|
poiru
:
review+
|
Details | Diff | Splinter Review |
1.24 KB,
patch
|
poiru
:
review+
|
Details | Diff | Splinter Review |
This will be used e.g. to create a variadic NS_IMPL_ISUPPORTS macro.
Assignee | ||
Comment 1•10 years ago
|
||
Try push: https://tbpl.mozilla.org/?tree=Try&rev=2ffb81605a43
Attachment #8398695 -
Flags: review?(jwalden+bmo)
Comment 2•10 years ago
|
||
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. :-)
Comment 3•10 years ago
|
||
This is similar to what i did in bug 900903, but that falls short on complex things.
Assignee | ||
Comment 4•10 years ago
|
||
(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)
Assignee | ||
Comment 5•10 years ago
|
||
Attachment #8399073 -
Attachment is obsolete: true
Attachment #8399073 -
Flags: review?(jwalden+bmo)
Attachment #8407751 -
Flags: review?(jwalden+bmo)
Assignee | ||
Comment 6•10 years ago
|
||
Attachment #8407753 -
Flags: review?(jwalden+bmo)
Assignee | ||
Comment 7•10 years ago
|
||
Try push: https://tbpl.mozilla.org/?tree=Try&rev=d902c631d381
Comment 8•10 years ago
|
||
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)
Comment 9•10 years ago
|
||
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 10•10 years ago
|
||
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+
Assignee | ||
Comment 11•10 years ago
|
||
(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 12•10 years ago
|
||
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+
Assignee | ||
Comment 13•10 years ago
|
||
Attachment #8408960 -
Attachment is obsolete: true
Attachment #8411966 -
Flags: review+
Assignee | ||
Comment 14•10 years ago
|
||
Attachment #8407753 -
Attachment is obsolete: true
Attachment #8411967 -
Flags: review+
Assignee | ||
Comment 15•10 years ago
|
||
Try push: https://tbpl.mozilla.org/?tree=Try&rev=629e2f114268
Keywords: checkin-needed
Comment 16•10 years ago
|
||
https://hg.mozilla.org/integration/mozilla-inbound/rev/c7399918527a https://hg.mozilla.org/integration/mozilla-inbound/rev/910de4b74638
Keywords: checkin-needed
Comment 17•10 years ago
|
||
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.
Comment 18•10 years ago
|
||
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
Assignee | ||
Comment 19•10 years ago
|
||
(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!
Updated•10 years ago
|
Attachment #8412665 -
Flags: review+
Assignee | ||
Updated•10 years ago
|
Keywords: checkin-needed
Assignee | ||
Comment 20•10 years ago
|
||
Fixed bug number.
Attachment #8412665 -
Attachment is obsolete: true
Attachment #8412712 -
Flags: review+
Comment 21•10 years ago
|
||
https://hg.mozilla.org/integration/mozilla-inbound/rev/fb880322be81
Keywords: checkin-needed
You need to log in
before you can comment on or make changes to this bug.
Description
•