ICU-22911 Fix coverity warning in numrange_fluent.cpp

[mike-fabian]

See: https://unicode-org.atlassian.net/browse/ICU-22911

Checklist

• Required: Issue filed: https://unicode-org.atlassian.net/browse/ICU-22911
• Required: The PR title must be prefixed with a JIRA Issue number.
• Required: The PR description must include the link to the Jira Issue, for example by completing the URL in the first checklist item
• Required: Each commit message must be prefixed with a JIRA Issue number.
• Issue accepted (done by Technical Committee after discussion)
• Tests included, if applicable
• API docs and/or User Guide docs changed or added, if applicable

[markusicu]

Good catch on use-after-move, but... now we are stealing from ourselves, and putting the stolen pointer right back where it came from??
Do we need to futz with the fAtomicFormatter at all in this move copy constructor? If that got moved, then "this" should have the right state, right?
If we do need to futz with it, then I suspect we need to do it before moving stuff from src.

@aheninger @sffc please chime in. I don't have context for how these objects are built.

[sffc]

I think that the pre-existing code is completely fine. fAtomicFormatter is a field of LNF but not NFS. We read only that field, not any other fields.

[markusicu]

@sffc coverity complains that we are using src after it's been subject to std::move. This seems like a fair complaint.

Could we std::move just the NFS slice of src?
Could we move the formatter before moving src?

[sffc]

I don't think it is a fair complaint. As far as I can tell, C++ does not offer a mechanism to mark only part of an object as having been moved (unlike Rust which provides such a mechanism). I found a thread that says that what this code is doing is the correct pattern when you have a child class calling the move constructor of a base class, although I don't know the credibility of the people responding on this thread:

https://cplusplus.com/forum/beginner/187808/

[sffc]

I think that the pre-existing code is completely fine. fAtomicFormatter is a field of LNF but not NFS. We read only that field, not any other fields.

[mike-fabian]

@sffc OK, so that means that the coverity warning is a false positive, right? That is reassuring.

But would my patch help to avoid the coverity warning, or is is wrong? Or should a cmment to suppress the coverity warning be added?

[sffc]

The patch as it currently stands is wrong; @markusicu explained why here: #3202 (comment)

It might be that there's a way to write this code that doesn't trigger the coverity warning, and doing so would be nice if possible. I'm not that familiar with how Coverity works.

[mike-fabian]

The patch as it currently stands is wrong; @markusicu explained why here: #3202 (comment)

It might be that there's a way to write this code that doesn't trigger the coverity warning, and doing so would be nice if possible. I'm not that familiar with how Coverity works.

I don't know much about coverity either. Adding this comment might remove the coverity warning:

--- a/icu4c/source/i18n/numrange_fluent.cpp
+++ b/icu4c/source/i18n/numrange_fluent.cpp
@@ -239,6 +239,7 @@ LocalizedNumberRangeFormatter::LocalizedNumberRangeFormatter(LocalizedNumberRang
 LocalizedNumberRangeFormatter::LocalizedNumberRangeFormatter(NFS<LNF>&& src) noexcept
         : NFS<LNF>(std::move(src)) {
     // Steal the compiled formatter
+    // coverity[use_after_move]
     LNF&& _src = static_cast<LNF&&>(src);
     auto* stolen = _src.fAtomicFormatter.exchange(nullptr);
     delete fAtomicFormatter.exchange(stolen);