ICU-22920 Avoid CTAD in Formattable's constructor. NFC

[Quuxplusone]

This line uses CTAD on pair, when every other place in the codebase uses a function call to make_pair (and no other place uses CTAD on any class template at all). Assume this was unintentional, and fix it.

warning: class template argument deduction is incompatible with
C++ standards before C++17; for compatibility, use explicit type
name 'std::pair<const Formattable *, int>'
(aka 'pair<const icu_77::message2::Formattable *, int>') [-Wctad]
      Formattable(const Formattable* arr, int32_t len) : contents(std::pair(arr, len)) {}
                                                                  ^~~~~~~~~

Checklist

• Required: Issue filed: ICU-NNNNN
• Required: The PR title must be prefixed with a JIRA Issue number. Example: "ICU-1234 Fix xyz"
• Required: Each commit message must be prefixed with a JIRA Issue number. Example: "ICU-1234 Fix xyz"
• Issue accepted (done by Technical Committee after discussion)
• Tests included, if applicable
• API docs and/or User Guide docs changed or added, if applicable

[CLAassistant]

All committers have signed the CLA.

[markusicu]

This does not seem to apply, since ICU requires C++17:

warning: class template argument deduction is incompatible with
C++ standards before C++17; for compatibility, use explicit type

[eggrobin]

every other place in the codebase uses a function call to make_pair (and no other place uses CTAD on any class template at all). Assume this was unintentional, and fix it.

We recently moved to requiring C++17, so older code won’t use these features, but consistency with legacy code is not a goal.

[Quuxplusone]

This does not seem to apply, since ICU requires C++17

Right, but given that every other place uses make_pair; and given that pair's behavior is different-and-less-desirable compared to make_pair, I personally think you should use make_pair here.

In this specific line, there is no difference between using pair and using make_pair. However, there are differences in general: for example, make_pair(ref(x), ref(y)) produces a pair of references, while pair(ref(x), ref(y)) produces a pair of reference_wrapper objects.

This kind of difference-in-behavior extends to other STL types as well; e.g. make_reverse_iterator(rit) always produces a reverse_iterator<decltype(rit)>, but the CTAD version reverse_iterator(rit) can select the copy deduction candidate instead, producing a non-reversed decltype(rit). See this post of mine (among others).

Basically I'm saying that there's light visible between the set of constructs that physically compile in C++17 mode, and the set of constructs that are a good idea to write in a C++17 codebase; and IMO CTAD falls into that gap: I agree that it physically does compile (just like the return-by-const in #3343 physically compiles), but its usage increases the maintenance burden rather than decreasing it. IMO the best style guideline re CTAD is "simply never use it." Adopting #3344 would cause ICU to adhere to that guideline, since this is ICU's solitary use of CTAD at the moment (and, like I said, I think it was basically a typo in this case).

(FYI: I noticed this solitary use when compiling ICU locally, only because I have configured my compiler to warn-by-default on any use of CTAD.)

[eggrobin]

IMO the best style guideline re CTAD is "simply never use it."

That may be your strongly-held opinion, but not necessarily ICU-TC’s—nor for that matter that of all of your fellow WG 21 experts (looking through the references you give, I come across https://reviews.llvm.org/D54565#1339926).

As you note, in this case (and most other practical cases), std::pair is fine, and I find it more readable than std::make_pair—and I also tend to like it on the containers. As for the std::ref example, if as a reviewer I have the misfortune of reading code with std::reference_wrapper, I would probably want explicit types anyway, instead of relying on knowing about the std::unwrap_ref_decay_t in std::make_pair.

[Quuxplusone]

As for the std::ref example, if as a reviewer I have the misfortune of reading code with std::reference_wrapper, I would probably want explicit types anyway

Oh, for sure, if this code had used : contents(std::pair<const Formattable*, int32_t>(arr, len)) then it would not have tripped my alarms, either. Anyway, feel free to close.

[markusicu]

I just found that internally to Google, we have a brand new C++ “Tip of the Week #238: Avoid std::make_pair and std::make_tuple”

It recommends using {a, b} if possible, otherwise std::pair(a, b) unless someone really understands and desires the std::decay behavior regarding reference_wrapper.