1143478 - Remove mozilla::Pair

Status: RESOLVED FIXED

(Core :: MFBT, defect)

Description

[Eric Rescorla (:ekr)]

std::pair is used throughout the Firefox code base (https://dxr.mozilla.org/mozilla-central/search?q=std%3A%3Apair&redirect=true), including in lots libraries which we pull from upstream and aren't going to change.

mozilla::pair is used almost nowhere
(https://dxr.mozilla.org/mozilla-central/search?q=%2Btype-ref%3Amozilla%3A%3APair)

I've heard claims that mozilla::pair exists to deal with implementation quality issues in std::pair (https://groups.google.com/forum/#!topic/mozilla.dev.platform/IQhGdhGxx2M) but as a practical matter, it seems clear that we've standardized on std::pair, so even if this is true, it's largely too late. I propose that we recognize the facts on the ground, remove mozilla::Pair, and recommend that people use the STL versions.

Comment 1

[Chris Fronk]

Attachment: Bug 1143478 - Rename mozilla::Pair to CompactPair. r=froydnj

Comment 2

[Chris Fronk]

Attachment: Bug 1143478 - Rename mozilla::Pair to CompactPair. r=froydnj

Comment 3

[Chris Fronk]

Not sure if this is still wanted but put at patch up to show what changes it would take. Used tuple for UniquePtr to keep the compiler asserts passing otherwise pretty straightforward

Comment 4

[Simon Giesecke [:sg] [he/him]]

Thanks for tackling this!

However, I am not sure if mozilla::Pair should be completely removed. I guess https://groups.google.com/d/msg/mozilla.dev.platform/IQhGdhGxx2M/NpEHE7OguoIJ sounds like good advice in summary to me. Paraphrasing that:

• In general, std::pair should be preferred over mozilla::Pair unless you need the empty type optimization.
• mozilla::Pair should be renamed to mozilla::CompressedPair to make the use case clearer.

Nathan, WDYT?

One more thing: you submitted two patches, which appear to do the same thing, but seem to be based on different base revisions? Could you abandon/mark one of those as obsolete?

Comment 5

[Simon Giesecke [:sg] [he/him]]

Sorry, I missed the final part (i.e. https://groups.google.com/d/msg/mozilla.dev.platform/IQhGdhGxx2M/NJ99NV4gmfcJ) saying that mozilla::Tuple (or does it refer to std::tuple?) offers this empty type optimization as well? Then, maybe we can get rid of mozilla::Pair entirely, and just check if there are cases that are better replaced by a tuple to benefit from empty type optimization.

Comment 6

[Nathan Froyd [:froydnj]]

(In reply to Simon Giesecke [:sg] [he/him] from comment #4)

Thanks for tackling this!

However, I am not sure if mozilla::Pair should be completely removed. I guess https://groups.google.com/d/msg/mozilla.dev.platform/IQhGdhGxx2M/NpEHE7OguoIJ sounds like good advice in summary to me. Paraphrasing that:

• In general, std::pair should be preferred over mozilla::Pair unless you need the empty type optimization.
• mozilla::Pair should be renamed to mozilla::CompressedPair to make the use case clearer.

Nathan, WDYT?

This works for me. I have a slight preference for CompactPair as the original post suggested instead of CompressedPair, but since the number of use cases is expected to be relatively small, I don't think it matters that much.

Comment 7

[Simon Giesecke [:sg] [he/him]]

(In reply to Nathan Froyd [:froydnj] from comment #6)

(In reply to Simon Giesecke [:sg] [he/him] from comment #4)

Thanks for tackling this!

However, I am not sure if mozilla::Pair should be completely removed. I guess https://groups.google.com/d/msg/mozilla.dev.platform/IQhGdhGxx2M/NpEHE7OguoIJ sounds like good advice in summary to me. Paraphrasing that:

• In general, std::pair should be preferred over mozilla::Pair unless you need the empty type optimization.
• mozilla::Pair should be renamed to mozilla::CompressedPair to make the use case clearer.

Nathan, WDYT?

This works for me. I have a slight preference for CompactPair as the original post suggested instead of CompressedPair, but since the number of use cases is expected to be relatively small, I don't think it matters that much.

Oh, this change was not intentional. Boost calls it boost::compressed_pair, but in fact I'd prefer CompactPair as well.

Comment 8

[Chris Fronk]

I've update the patch to rename to CompactPair and updated UniquePtr WasmOpIter.h back to use CompactPair. Let me know if there's any others you think should be switched back. Sorry for confusion with revision it updated the old one and not sure how to add reviewers back.

Comment 9

[Pulsebot]

Pushed by [e-mail]:
https://hg.mozilla.org/integration/autoland/rev/a08637fb30c8
Rename mozilla::Pair to CompactPair. r=froydnj

Comment 10

[Stefan Hindli [:stefan_hindli]]

Backed out changeset a08637fb30c8 (Bug 1143478) for causing bustages in /builds/worker/checkouts/gecko/ipc/mscom/Registration.cpp CLOSED TREE

https://treeherder.mozilla.org/#/jobs?repo=autoland&resultStatus=testfailed%2Cbusted%2Cexception&classifiedState=unclassified&fromchange=2883af6562be0036caa8364ef37536e86f08c0b9&selectedJob=292668381

https://treeherder.mozilla.org/logviewer.html#/jobs?job_id=292668381&repo=autoland&lineNumber=8633

Comment 11

[Pulsebot]

Backout by [e-mail]:
https://hg.mozilla.org/integration/autoland/rev/7a6232e87e8f
Backed out changeset a08637fb30c8 for causing bustages in /builds/worker/checkouts/gecko/ipc/mscom/Registration.cpp CLOSED TREE

Comment 12

[Pulsebot]

Pushed by [e-mail]:
https://hg.mozilla.org/integration/autoland/rev/4db022e14a60
Rename mozilla::Pair to CompactPair. r=froydnj

Comment 13

[Bogdan Tara[:bogdan_tara | bogdant]]

https://hg.mozilla.org/mozilla-central/rev/4db022e14a60

Comment 14

[Chris Fronk]

This is just a commonly-used header causing sccache invalidations.

I think Nathan (froydnj) commented here already with above but if this is still issue or you need more info from me let me know