Closed Bug 1059038 Opened 5 years ago Closed 5 years ago

Move mozilla::unused to MFBT

Categories

(Core :: MFBT, defect)

defect
Not set

Tracking

()

RESOLVED FIXED
mozilla35

People

(Reporter: jld, Assigned: jld)

Details

Attachments

(1 file)

mozilla::unused could be in MFBT, but currently it's in xpcom/glue.  This means that code in separate libraries not linked against xpcom code can't use it (or, more specifically, that trying to use it will break no-opt builds only, a case which isn't currently tested by buildbot, because the reference to the mozilla::unused symbol is normally optimized out).  That should be fixed.
This is… not trivial.  Making that change means that everything that uses mozilla::unused needs to USE_LIBS mfbt, instead of just including the headers, and apparently things mostly don't do that already.
I wonder whether it would be practical to include mfbt in the xpcomglue libraries.
(In reply to Jed Davis [:jld] from comment #1)
> This is… not trivial.  Making that change means that everything that uses
> mozilla::unused needs to USE_LIBS mfbt, instead of just including the
> headers, and apparently things mostly don't do that already.

Most things are linked against mozglue, which includes mfbt. What is the problem you're seeing exactly?
(In reply to Mike Hommey [:glandium] (out from Sep 6 to Sep 22) from comment #3)
> (In reply to Jed Davis [:jld] from comment #1)
> > This is… not trivial.  Making that change means that everything that uses
> > mozilla::unused needs to USE_LIBS mfbt, instead of just including the
> > headers, and apparently things mostly don't do that already.

I blame either too much coffee or too little for this comment.

> Most things are linked against mozglue, which includes mfbt. What is the
> problem you're seeing exactly?

The opposite of bug 1059602, almost — mozilla::unused needs to be MFBT_DATA, so that this happens:

$ nm -D libxul.so | grep unused
                 w _ZN7mozilla6unusedE
$ nm -D firefox | grep unused
000000000043a14f R _ZN7mozilla6unusedE
https://tbpl.mozilla.org/?tree=Try&rev=31f8da89c927 (note that patch has a typo in the commit message; it's fixed in the copy I'm attaching)

unused.h is both moved and edited to adjust its linkage attributes.
Attachment #8481414 - Flags: review?(mh+mozilla)
Comment on attachment 8481414 [details] [diff] [review]
bug1059038-move-unused-hg0.diff

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

r=me on a purely technical level. But at the code style level, I think the files and types need a rename to begin with a capital U. Waldo will tell for sure.
Attachment #8481414 - Flags: review?(mh+mozilla)
Attachment #8481414 - Flags: review?(jwalden+bmo)
Attachment #8481414 - Flags: review+
Attachment #8481414 - Flags: review?(jwalden+bmo) → review+
Re the naming thing, I think probably this should just stay a one-off exception.  I think as a reader I'd have slightly more questions if I saw "Unused << ..." than if I saw "unused << ...", and we don't want any questions here, if we can help it.

Honestly I don't really like this pattern at all, and I think people should 1) not ignore return values, at the least to assert some characteristic about them, or failing that, 2) just cast to void.  If really-old gcc thinks cast to void is not enough to suppress unused return values, that's its problem, and we should just turn off the warning for such compilers.  But it's not worth the argument, at least not until such gcc is dead and we can revert to a cast without needing to flip off a warning.
https://hg.mozilla.org/mozilla-central/rev/dc971e50ebf0
Status: NEW → RESOLVED
Closed: 5 years ago
Resolution: --- → FIXED
Target Milestone: --- → mozilla35
You need to log in before you can comment on or make changes to this bug.