1059038 - Move mozilla::unused to MFBT

Status: RESOLVED FIXED

(Core :: MFBT, defect)

Description

[Jed Davis [:jld] ⟨⏰|UTC-7⟩ ⟦he/him⟧]

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.

Comment 1

[Jed Davis [:jld] ⟨⏰|UTC-7⟩ ⟦he/him⟧]

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.

Comment 2

[Karl Tomlinson (:karlt)]

I wonder whether it would be practical to include mfbt in the xpcomglue libraries.

Comment 3

[Mike Hommey [:glandium]]

(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?

Comment 4

[Jed Davis [:jld] ⟨⏰|UTC-7⟩ ⟦he/him⟧]

(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

Comment 5

[Jed Davis [:jld] ⟨⏰|UTC-7⟩ ⟦he/him⟧]

Attachment: bug1059038-move-unused-hg0.diff

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.

Comment 6

[Mike Hommey [:glandium]]

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.

Comment 7

[Jeff Walden [:Waldo]]

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.

Comment 8

[Jed Davis [:jld] ⟨⏰|UTC-7⟩ ⟦he/him⟧]

https://tbpl.mozilla.org/?tree=Try&rev=872aa16a1b63

Comment 9

[Carsten Book [:Tomcat]]

https://hg.mozilla.org/integration/mozilla-inbound/rev/dc971e50ebf0

Comment 10

[Carsten Book [:Tomcat]]

https://hg.mozilla.org/mozilla-central/rev/dc971e50ebf0