Closed Bug 1143478 Opened 6 years ago Closed 6 months ago

Remove mozilla::Pair

Categories

(Core :: MFBT, defect)

38 Branch
x86
macOS
defect
Not set
normal

Tracking

()

RESOLVED FIXED
mozilla76
Tracking Status
firefox76 --- fixed

People

(Reporter: ekr, Assigned: fronkc1)

References

Details

Attachments

(1 file, 1 obsolete file)

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.
Assignee: nobody → fronkc1
Status: NEW → ASSIGNED

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

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?

Flags: needinfo?(nfroyd)
Flags: needinfo?(fronkc1)

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.

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

Flags: needinfo?(nfroyd)

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

Attachment #9129377 - Attachment is obsolete: true
Attachment #9129377 - Attachment description: Bug 1143478 - Remove mozilla::Pair. r=froydnj → Bug 1143478 - Rename mozilla::Pair to CompactPair. r=froydnj
Attachment #9129377 - Attachment is obsolete: false
Attachment #9129376 - Attachment is obsolete: true

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.

Flags: needinfo?(fronkc1)
Attachment #9129376 - Attachment is obsolete: false
Attachment #9129376 - Attachment description: Bug 1143478 - Remove mozilla::Pair. r=froydnj → Bug 1143478 - Rename mozilla::Pair to CompactPair. r=froydnj
Attachment #9129377 - Attachment is obsolete: true
Pushed by nfroyd@mozilla.com:
https://hg.mozilla.org/integration/autoland/rev/a08637fb30c8
Rename mozilla::Pair to CompactPair. r=froydnj
Backout by shindli@mozilla.com:
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
Pushed by nfroyd@mozilla.com:
https://hg.mozilla.org/integration/autoland/rev/4db022e14a60
Rename mozilla::Pair to CompactPair. r=froydnj
Status: ASSIGNED → RESOLVED
Closed: 6 months ago
Resolution: --- → FIXED
Target Milestone: --- → mozilla76
Regressions: 1623362

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

Flags: needinfo?(fronkc1)
You need to log in before you can comment on or make changes to this bug.