1603127 - Replace mozilla::Tie with structured bindings where appropriate in mozilla::Encoding/Decoder/Encoder usage

Status: RESOLVED FIXED

(Core :: Internationalization, task, P3)

Description

[Henri Sivonen (:hsivonen)]

Various things in mozilla/Encoding.h return mozilla::Tuple. Now that we're on C++17, review the callers to see which call sites would become more readable if migrated to structured bindings.

Comment 1

[Shazib Summar [:ssummar]]

Hi :hsivonen, do you mean to change all std::tie usages to structured bindings in all files where a function defined in Encoding.h is used? Or am I misunderstanding? Can you kindly elaborate this. Thank you.

Comment 2

[Henri Sivonen (:hsivonen)]

I mean using structured bindings on all call sites that now call methods from mozilla/Encoding.h that now return mozilla::Tuple.

Comment 3

[David Parks [:handyman]]

Shazib is working on this as a good-first-bug, and discovered that mozilla::Tuple doesn't support structured bindings (previously reported as bug 1633345). I then wrote a patch for that, and I could submit it there but I also found the (much older) bug 1276351 that suggests we should just migrate APIs away from mozilla:Tuple and instead use std::tuple. I'm thinking that using std::tuple might be the better approach for this bug. Your thoughts/preferences?

Comment 4

[Henri Sivonen (:hsivonen)]

Botond, is there a reason to stay with mozilla::Tuple or should we migrate to std::tuple?

Comment 5

[Botond Ballo [:botond]]

(In reply to Henri Sivonen (:hsivonen) from comment #4)

Botond, is there a reason to stay with mozilla::Tuple or should we migrate to std::tuple?

I'm supportive of migrating to std::tuple.

I tried it a few years ago, and at the time a major sticking point was that nsAutoPtr could not be stored in std::tuple (and there was a lot of code passing nsAutoPtr to runnable wrappers, whose implementation involved storing the arguments in a tuple). Since then, I see that nsAutoPtr has been removed in bug 1610067, so hopefully a migration should be much smoother now.

Comment 6

[Shazib Summar [:ssummar]]

Thanks :botond. I'll be working on migrating to std::tuple then. You can expect a patch in a few days.

Comment 7

[Shazib Summar [:ssummar]]

Attachment: Bug 1603127 - Replaced mozilla::Tuple with std::tuple and applied structured bindings in mozilla/Encoding.h. r=hsivonen

Comment 8

[Pulsebot]

Pushed by [e-mail]:
https://hg.mozilla.org/integration/autoland/rev/7e8e3747c3f8
Replaced mozilla::Tuple with std::tuple and applied structured bindings in mozilla/Encoding.h. r=hsivonen

Comment 9

[Shazib Summar [:ssummar]]

Thanks :)

Comment 10

[Cristian Tuns]

https://hg.mozilla.org/mozilla-central/rev/7e8e3747c3f8

Comment 11

[Cristian Tuns]

Backed out for causing toolchains build bustages (Bug 1739589)

• Backout link
• Push with failures
• Failure Log

Comment 12

[Shazib Summar [:ssummar]]

(In reply to Cristian Tuns from comment #11)

Backed out for causing toolchains build bustages (Bug 1739589)

• Backout link
• Push with failures
• Failure Log

Sorry about this. I didnt know non-debug and debug builds are different. I only tested on debug builds. I'm working on fixing this.

Comment 13

[Pulsebot]

Pushed by [e-mail]:
https://hg.mozilla.org/comm-central/rev/b2e956b4671e
use std::tie instead of mozilla::Tie in comm-central. rs=bustage-fix DONTBUILD

Comment 14

[Pulsebot]

Backout by [e-mail]:
https://hg.mozilla.org/mozilla-central/rev/eb905e7bd8d2
Backed out changeset 7e8e3747c3f8 for causing toolchains build bustages (Bug 1739589). CLOSED TREE

Comment 15

[Pulsebot]

Backout by [e-mail]:
https://hg.mozilla.org/comm-central/rev/636157cf21e4
Backed out comm-central changeset b2e956b4671e since m-c fix was backed out. rs=bustage-fix DONTBUILD

Comment 16

[Shazib Summar [:ssummar]]

Fixed in latest revision. Hopefully won't need anymore revs.

Comment 17

[Pulsebot]

Pushed by [e-mail]:
https://hg.mozilla.org/integration/autoland/rev/f861c1ac64f0
Replaced mozilla::Tuple with std::tuple and applied structured bindings in mozilla/Encoding.h. r=hsivonen

Comment 18

[Narcis Beleuzu [:NarcisB]]

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

Comment 19

[Pulsebot]

Pushed by [e-mail]:
https://hg.mozilla.org/comm-central/rev/9a070c9b7ccd
use std::tie instead of mozilla::Tie in comm-central. rs=bustage-fix DONTBUILD