Closed Bug 900903 Opened 6 years ago Closed 6 years ago

Avoid using numbered macros for cycle collector macros

Categories

(Core :: XPCOM, defect)

defect
Not set

Tracking

()

RESOLVED FIXED
mozilla31

People

(Reporter: glandium, Assigned: poiru)

References

Details

Attachments

(3 files, 5 obsolete files)

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)
Attachment #784926 - Flags: review?(jwalden+bmo)
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.
For whichever want to take it.
Attachment #784927 - Flags: review?(continuation)
Attachment #784927 - Flags: review?(bugs)
Blocks: 900908
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)
Given we have uses with 17 args already, silent fail at 20 is pretty scary...  Can we somehow make the fail non-silent?  :(
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
Ah, it is in the first patch
With CC stuff, we need some explicit error if there are more than 20 fields to traverse/unlink
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-
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)
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)
(Note 20 is an arbitrary limit i took because i saw the current max was 17. I could very well make it more)
This is fantastic!
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+
Great work! As a follow-up, there are more macros in nsWrapperCache.h that also need this treatment.
(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.
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
By the way, does anyone know if we will soon be able to assume variadic template support?
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: mh+mozilla → birunthan
Status: NEW → ASSIGNED
Depends on: 989460
No longer blocks: 900908
Attachment #784927 - Attachment is obsolete: true
Attachment #8398807 - Attachment is obsolete: true
Attachment #8407756 - Flags: review?(bugs)
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}
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)
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 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+
Try push: https://tbpl.mozilla.org/?tree=Try&rev=629e2f114268
Keywords: checkin-needed
Whiteboard: [leave open]
Version: 24 Branch → Trunk
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+
Attachment #8412863 - Flags: review?(bugs) → review+
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+
Keywords: checkin-needed
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]
Attachment #8412862 - Flags: checkin+
Attachment #8412863 - Flags: checkin+
https://hg.mozilla.org/mozilla-central/rev/a9a093d2881a
https://hg.mozilla.org/mozilla-central/rev/9baacd7d775f
Status: ASSIGNED → RESOLVED
Closed: 6 years ago
Resolution: --- → FIXED
Target Milestone: --- → mozilla31
You need to log in before you can comment on or make changes to this bug.