icu
icu copied to clipboard
unicode-org
ICU-22920 Avoid CTAD in Formattable's constructor. NFC
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
- [x] Required: Issue filed: ICU-NNNNN
- [x] Required: The PR title must be prefixed with a JIRA Issue number. Example: "ICU-1234 Fix xyz"
- [x] 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
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
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.
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.)
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.
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.
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.