This is an archive of the discontinued LLVM Phabricator instance.

[libcxx][NFC] tidy up money_get::__do_get's sign parsing
ClosedPublic

Authored by DanielMcIntosh-IBM on Nov 1 2021, 1:47 PM.

Details

Summary

Same logic, but much easier to read this way

Diff Detail

Event Timeline

DanielMcIntosh-IBM requested review of this revision.Nov 1 2021, 1:47 PM
DanielMcIntosh-IBM created this revision.
Herald added a project: Restricted Project. · View Herald TranscriptNov 1 2021, 1:47 PM
Herald added a reviewer: Restricted Project. · View Herald Transcript
Mordante accepted this revision as: Mordante.Nov 2 2021, 11:40 AM
Mordante added a subscriber: Mordante.

Thanks, this indeed looks better readable!
LGTM, but I prefer to remove the unneeded assignment.

libcxx/include/locale
2898

The original algorithm never sets __neg to false. Looking at the code it seems the call-site always sets this value to false.

libcxx/include/locale
2898

True, however, as you mentioned, the call-site always sets the value to false, so this isn't actually a functional change.

I'm open to other ideas, but explicitly setting __neg to false when we detect a positive seems more illustrative to me, and generally safer.

Additionally, explicitly setting it to false here means that the only case where we _don't_ set __neg is when __psn.size() == 0 && __nsn.size() == 0 - i.e. when the locale has no means of specifying a positive or negative value. This lets us use the initial value of __neg as a default value for that case.

Add comment further explaining the behaviour of __neg

ldionne accepted this revision.Nov 3 2021, 10:01 AM
This revision is now accepted and ready to land.Nov 3 2021, 10:01 AM
Mordante accepted this revision.Nov 3 2021, 11:01 AM

LGTM!

libcxx/include/locale
2898

Fair points.

DanielMcIntosh-IBM marked an inline comment as done.Nov 4 2021, 2:58 PM