Closed
Bug 900903
Opened 11 years ago
Closed 10 years ago
Avoid using numbered macros for cycle collector macros
Categories
(Core :: XPCOM, defect)
Core
XPCOM
Tracking
()
RESOLVED
FIXED
mozilla31
People
(Reporter: glandium, Assigned: poiru)
References
Details
Attachments
(3 files, 5 obsolete files)
74.34 KB,
patch
|
poiru
:
review+
poiru
:
checkin+
|
Details | Diff | Splinter Review |
83.00 KB,
patch
|
smaug
:
review+
RyanVM
:
checkin+
|
Details | Diff | Splinter Review |
5.64 KB,
patch
|
smaug
:
review+
RyanVM
:
checkin+
|
Details | Diff | Splinter Review |
So, instead of doing NS_IMPL_CYCLE_COLLECTION_1(foo, bar) or NS_IMPL_CYCLE_COLLECTION_5(foo, bar, baz, qux, hoge, piyo), one would do NS_IMPL_CYCLE_COLLECTION(foo, bar) or NS_IMPL_CYCLE_COLLECTION(foo, bar, baz, qux, hoge, piyo)
Reporter | ||
Comment 1•11 years ago
|
||
Attachment #784926 -
Flags: review?(jwalden+bmo)
Comment 2•11 years ago
|
||
Comment on attachment 784926 [details] [diff] [review] Move some MFBT macros in a new header and make them more generic Review of attachment 784926 [details] [diff] [review]: ----------------------------------------------------------------- ::: mfbt/MacroHelpers.h @@ +4,5 @@ > + * file, You can obtain one at http://mozilla.org/MPL/2.0/. */ > + > +/* A set of preprocessor macros helpers */ > + > +#ifndef mozilla_MacroHelpers_h_ No trailing underscore. Which reminds me, I need to change STYLE.
Reporter | ||
Comment 3•11 years ago
|
||
For whichever want to take it.
Attachment #784927 -
Flags: review?(continuation)
Attachment #784927 -
Flags: review?(bugs)
Reporter | ||
Comment 4•11 years ago
|
||
Comment on attachment 784926 [details] [diff] [review] Move some MFBT macros in a new header and make them more generic The MOZ_COUNT_ARGS replacement is too subtle for MOZ_ASSERT :(
Attachment #784926 -
Flags: review?(jwalden+bmo)
Comment 5•11 years ago
|
||
Given we have uses with 17 args already, silent fail at 20 is pretty scary... Can we somehow make the fail non-silent? :(
Comment 6•11 years ago
|
||
Comment on attachment 784927 [details] [diff] [review] Use varadic macros for NS_IMPL_CYCLE_COLLECTION and NS_IMPL_CYCLE_COLLECTION_INHERITED -NS_IMPL_CYCLE_COLLECTION_6(nsFindContentIterator, mOuterIterator, mInnerIterator, +NS_IMPL_CYCLE_COLLECTION(nsFindContentIterator, mOuterIterator, mInnerIterator, mStartOuterContent, mEndOuterContent, mEndNode, mStartNode) Align the params What is MOZ_CHOOSE_HELPER? I don't see it in mxr nor in this patch
Comment 7•11 years ago
|
||
Ah, it is in the first patch
Comment 8•11 years ago
|
||
With CC stuff, we need some explicit error if there are more than 20 fields to traverse/unlink
Comment 9•11 years ago
|
||
Comment on attachment 784927 [details] [diff] [review] Use varadic macros for NS_IMPL_CYCLE_COLLECTION and NS_IMPL_CYCLE_COLLECTION_INHERITED r- because of the missing 20+ fields error reporting.
Attachment #784927 -
Flags: review?(continuation)
Attachment #784927 -
Flags: review?(bugs)
Attachment #784927 -
Flags: review-
Reporter | ||
Comment 10•11 years ago
|
||
Comment on attachment 784927 [details] [diff] [review] Use varadic macros for NS_IMPL_CYCLE_COLLECTION and NS_IMPL_CYCLE_COLLECTION_INHERITED Beyond 20 params, it actually errors out: error: 'NS_IMPL_CYCLE_COLLECTION_UNLINK_u' was not declared in this scope NS_IMPL_CYCLE_COLLECTION_UNLINK(a,b,c,d,e,f,g,h,i,j,k,l,m,n,o,p,q,r,s,t,u,v,w,x,y,z)
Attachment #784927 -
Flags: review- → review?(bugs)
Reporter | ||
Comment 11•11 years ago
|
||
And in case the 21st argument is a number: error: macro "NS_IMPL_CYCLE_COLLECTION_UNLINK_1" passed 26 arguments, but takes just 1 NS_IMPL_CYCLE_COLLECTION_UNLINK(a,b,c,d,e,f,g,h,i,j,k,l,m,n,o,p,q,r,s,t,1,v,w,x,y,z)
Reporter | ||
Comment 12•11 years ago
|
||
(Note 20 is an arbitrary limit i took because i saw the current max was 17. I could very well make it more)
Comment 13•11 years ago
|
||
Neat!
Comment 14•11 years ago
|
||
This is fantastic!
Comment 15•11 years ago
|
||
Comment on attachment 784927 [details] [diff] [review] Use varadic macros for NS_IMPL_CYCLE_COLLECTION and NS_IMPL_CYCLE_COLLECTION_INHERITED I assume you're tweaking the other patch to give the better error reporting you meantioned on IRC.
Attachment #784927 -
Flags: review?(bugs) → review+
Comment 16•11 years ago
|
||
Great work! As a follow-up, there are more macros in nsWrapperCache.h that also need this treatment.
Reporter | ||
Comment 17•11 years ago
|
||
(In reply to Benoit Jacob [:bjacob] from comment #16) > Great work! As a follow-up, there are more macros in nsWrapperCache.h that > also need this treatment. iirc that's handled by my wip for bug 900908, but it's kind of blocked by MacroHelpers not working properly for both gcc and msvc at the same time.
Reporter | ||
Comment 18•10 years ago
|
||
FTR, this is the last patch I wrote, in case someone wants to try harder (and it looks like :poiru wants to)
Attachment #784926 -
Attachment is obsolete: true
Comment 19•10 years ago
|
||
By the way, does anyone know if we will soon be able to assume variadic template support?
Assignee | ||
Comment 20•10 years ago
|
||
Assigning myself. I have patches for this that work fine on all our compilers (see https://tbpl.mozilla.org/?tree=Try&rev=fbf2c396891f), but I'll wait for bug 989460 to land before attaching them here.
Assignee | ||
Comment 21•10 years ago
|
||
Attachment #784927 -
Attachment is obsolete: true
Attachment #8398807 -
Attachment is obsolete: true
Attachment #8407756 -
Flags: review?(bugs)
Assignee | ||
Updated•10 years ago
|
Attachment #8407756 -
Attachment description: Rename NS_IMPL_CYCLE_COLLECTION_{0 => GRAPH_ONLY, UNLINK_0 => EMPTY_UNLINK} → Part 1: Rename NS_IMPL_CYCLE_COLLECTION_{0 => GRAPH_ONLY, UNLINK_0 => EMPTY_UNLINK}
Assignee | ||
Comment 22•10 years ago
|
||
By using the MOZ_FOR_EACH macro (see bug 989460), the numbered CC macros can be simplified into just a few short and sweet variadic macros. The old numbered macros are still present, but simply forward to the variadic macro. I'll convert everything to use the variadic macros and remove the numbered stubs after this patch lands. Using the variadic macros with too few or too many arguments will result in a compile-time error. Right now, MacroArgs.h/MacroForEach.h support up to 50 arguments.
Attachment #8407784 -
Flags: review?(bugs)
Assignee | ||
Comment 23•10 years ago
|
||
Try push: https://tbpl.mozilla.org/?tree=Try&rev=d902c631d381
Comment 24•10 years ago
|
||
Comment on attachment 8407756 [details] [diff] [review] Part 1: Rename NS_IMPL_CYCLE_COLLECTION_{0 => GRAPH_ONLY, UNLINK_0 => EMPTY_UNLINK} Actually, I think we don't need this at all. The more I think, the more I prefer _0 over _EMPTY_UNLINK or _GRAPH_ONLY. The current macros have better consistency.
Attachment #8407756 -
Flags: review?(bugs)
Comment 25•10 years ago
|
||
Comment on attachment 8407784 [details] [diff] [review] Part 2: Add variadic variants of numbered macros in nsCycleCollectionParticipant.h Assuming MOZ_FOR_EACH works the way I think. It isn't in the tree yet.
Attachment #8407784 -
Flags: review?(bugs) → review+
Assignee | ||
Comment 26•10 years ago
|
||
Attachment #8407756 -
Attachment is obsolete: true
Attachment #8407784 -
Attachment is obsolete: true
Attachment #8411989 -
Flags: review+
Assignee | ||
Comment 27•10 years ago
|
||
Try push: https://tbpl.mozilla.org/?tree=Try&rev=629e2f114268
Comment 28•10 years ago
|
||
https://hg.mozilla.org/integration/mozilla-inbound/rev/bd83aff8800a
Keywords: checkin-needed
Assignee | ||
Updated•10 years ago
|
Attachment #8411989 -
Attachment description: Add variadic variants of numbered macros in nsCycleCollectionParticipant.h → Part 1: Add variadic variants of numbered macros in nsCycleCollectionParticipant.h
Attachment #8411989 -
Flags: checkin+
Assignee | ||
Comment 30•10 years ago
|
||
Attachment #8412862 -
Flags: review?(bugs)
Assignee | ||
Comment 31•10 years ago
|
||
Try push: https://tbpl.mozilla.org/?tree=Try&rev=0717302deed9
Attachment #8412863 -
Flags: review?(bugs)
Updated•10 years ago
|
Attachment #8412863 -
Flags: review?(bugs) → review+
Comment 32•10 years ago
|
||
Comment on attachment 8412862 [details] [diff] [review] Part 2: Change uses of numbered macros in nsCycleCollectionParticipant.h to the variadic variants In general -U 8 -p patches are preferred.
Attachment #8412862 -
Flags: review?(bugs) → review+
Assignee | ||
Updated•10 years ago
|
Keywords: checkin-needed
Comment 33•10 years ago
|
||
https://hg.mozilla.org/integration/mozilla-inbound/rev/a9a093d2881a https://hg.mozilla.org/integration/mozilla-inbound/rev/9baacd7d775f Given Monday's uplift, it's probably best for tracking purposes to let this bug be resolved when these 2 csets merge to m-c and file a new bug for any other work that still needs to be done. If you disagree, feel free to re-add the leave-open.
Keywords: checkin-needed
Whiteboard: [leave open]
Updated•10 years ago
|
Attachment #8412862 -
Flags: checkin+
Updated•10 years ago
|
Attachment #8412863 -
Flags: checkin+
Comment 34•10 years ago
|
||
https://hg.mozilla.org/mozilla-central/rev/a9a093d2881a https://hg.mozilla.org/mozilla-central/rev/9baacd7d775f
Status: ASSIGNED → RESOLVED
Closed: 10 years ago
Resolution: --- → FIXED
Target Milestone: --- → mozilla31
You need to log in
before you can comment on or make changes to this bug.
Description
•