ICU-22910 Fix coverity warning in number_fluent.cpp #3201
Conversation
| lnfMoveHelper(static_cast<LNF&&>(src)); // Call before moving src | ||
| static_cast<NFS<LNF>&>(*this) = std::move(src); // Move after |
There was a problem hiding this comment.
Good catch on use-after-move, but I don't know enough about how these objects are handled.
Why do we need both the NFS move construction and also the lnfMoveHelper?
@sffc @aheninger please help.
There was a problem hiding this comment.
LocalizedNumberFormatter inherits from NumberFormatterSettings, and it calls the move constructor of the base class. This PR is already questionable in the sense that it changes a move constructor to a move assignment.
lnfMoveHelper exists in order to deduplicate code between the move constructor and the move assignment operator. However, lnfMoveHelper assumes that the type is properly initialized: it reads and writes fields. So, I think it is only correct to call that function after a call to another constructor or assignment operator.
That said, it does appear that it's wrong to read from a moved value. However, it appears that it is legal to do this so long as the only fields read are the ones that belong to the child class and not the base class:
https://cplusplus.com/forum/beginner/187808/
It might be that cleaner code would be to change the helper function to take positional arguments of the fields from src instead of an rvalue reference that can't exist at the time the function is called.
| lnfMoveHelper(static_cast<LNF&&>(src)); // Call before moving src | ||
| static_cast<NFS<LNF>&>(*this) = std::move(src); // Move after |
There was a problem hiding this comment.
LocalizedNumberFormatter inherits from NumberFormatterSettings, and it calls the move constructor of the base class. This PR is already questionable in the sense that it changes a move constructor to a move assignment.
lnfMoveHelper exists in order to deduplicate code between the move constructor and the move assignment operator. However, lnfMoveHelper assumes that the type is properly initialized: it reads and writes fields. So, I think it is only correct to call that function after a call to another constructor or assignment operator.
That said, it does appear that it's wrong to read from a moved value. However, it appears that it is legal to do this so long as the only fields read are the ones that belong to the child class and not the base class:
https://cplusplus.com/forum/beginner/187808/
It might be that cleaner code would be to change the helper function to take positional arguments of the fields from src instead of an rvalue reference that can't exist at the time the function is called.
See: https://unicode-org.atlassian.net/browse/ICU-22910
Checklist