Skip to content

CFNumber: add missing break statements in CFNumberGetType#57

Open
DTW-Thalion wants to merge 2 commits into
gnustep:masterfrom
DTW-Thalion:fix/cfnumber-gettype-fallthrough
Open

CFNumber: add missing break statements in CFNumberGetType#57
DTW-Thalion wants to merge 2 commits into
gnustep:masterfrom
DTW-Thalion:fix/cfnumber-gettype-fallthrough

Conversation

@DTW-Thalion

Copy link
Copy Markdown
Contributor

Summary

The objCType switch in CFNumberGetType (the bridged-NSNumber path,
CF_IS_OBJC) has no break statements, so every case falls through to
the last (kCFNumberDoubleType). A bridged NSNumber of any type therefore
reports kCFNumberDoubleType, which also gives wrong results for
CFNumberGetByteSize, CFNumberIsFloatType, and equality of bridged numbers.

Fix

Add the missing break after each case.

Test

Tests/CFNumber/gettype.m: an integer NSNumber must report
kCFNumberIntType. It returns kCFNumberDoubleType before this change and the
correct type after.

(A single integer case is used deliberately: gnustep-base represents small
long/short/char numbers with an int objCType, and float/double
NSNumbers are tagged pointers that don't bridge through the CF FFI path — so
int is the one case that exercises this switch portably.)

The objCType switch in CFNumberGetType (the bridged-NSNumber path) had
no break statements, so every case fell through to the last and a
bridged NSNumber of any type reported kCFNumberDoubleType.  This also
gave wrong results for CFNumberGetByteSize, CFNumberIsFloatType and
equality of bridged numbers.

Add Tests/CFNumber/gettype.m.
Comment thread Tests/CFNumber/gettype.m Outdated
/* CFNumberGetType on a bridged NSNumber must reflect the number's actual
* type. The objCType switch was missing its break statements, so every
* case fell through to kCFNumberDoubleType; an integer NSNumber wrongly
* reported kCFNumberDoubleType. */

Copy link
Copy Markdown

Choose a reason for hiding this comment

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

Remove the comment

Copy link
Copy Markdown
Contributor Author

Choose a reason for hiding this comment

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

Done in fbd8ec7.

@HendrikHuebner HendrikHuebner left a comment

Copy link
Copy Markdown

Choose a reason for hiding this comment

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

LGTM

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

None yet

Development

Successfully merging this pull request may close these issues.

2 participants