Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

Sign up for GitHub

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Jump to bottom

ICU-22910 Fix coverity warning in number_fluent.cpp #3201

Open
wants to merge 1 commit into
base: main
Choose a base branch
from
Open

ICU-22910 Fix coverity warning in number_fluent.cpp #3201

Changes from all commits
Commits
Show all changes
1 commit
Select commit
File filter

Filter by extension

Filter by extension
Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
6 changes: 3 additions & 3 deletions icu4c/source/i18n/number_fluent.cpp
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -472,9 +472,9 @@ LocalizedNumberFormatter::LocalizedNumberFormatter(const NFS<LNF>& other)
LocalizedNumberFormatter::LocalizedNumberFormatter(LocalizedNumberFormatter&& src) noexcept
: LNF(static_cast<NFS<LNF>&&>(src)) {}

LocalizedNumberFormatter::LocalizedNumberFormatter(NFS<LNF>&& src) noexcept
: NFS<LNF>(std::move(src)) {
lnfMoveHelper(std::move(static_cast<LNF&&>(src)));
LocalizedNumberFormatter::LocalizedNumberFormatter(NFS<LNF>&& src) noexcept {
lnfMoveHelper(static_cast<LNF&&>(src)); // Call before moving src
static_cast<NFS<LNF>&>(*this) = std::move(src); // Move after
Comment on lines +476 to +477
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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.

Copy link
Member

@sffc sffc Sep 24, 2024 •
edited
Loading

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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.

}

LocalizedNumberFormatter& LocalizedNumberFormatter::operator=(const LNF& other) {
Expand Down