Opened 3 years ago

Closed 13 months ago

#12824 closed bug (fixed)

[Patch] partitioning_systems/gpt/utility.cpp: Fix clang warnings

Reported by: mt Owned by: jessicah
Priority: normal Milestone: Unscheduled
Component: Partitioning Systems/GPT Version: R1/Development
Keywords: Cc:
Blocked By: Blocking:
Has a Patch: no Platform: All

Description

In to_utf8(), clang warns '-Wtautological-constant-out-of-range-compare warnings'. I think we may increase variable 'c' size from uint16 to uint32 at line 41.

/home/haiku/haiku/haiku/src/add-ons/kernel/partitioning_systems/gpt/utility.cpp:50:16: warning: comparison of constant 65536 with expression of type 'uint16' (aka 'unsigned short') is always true [-Wtautological-constant-out-of-range-compare]
                } else if (c < 0x10000) {
                           ~ ^ ~~~~~~~
/home/haiku/haiku/haiku/src/add-ons/kernel/partitioning_systems/gpt/utility.cpp:54:16: warning: comparison of constant 1114111 with expression of type 'uint16' (aka 'unsigned short') is always true [-Wtautological-constant-out-of-range-compare]
                } else if (c <= 0x10ffff) {
                           ~ ^  ~~~~~~~~

Attachments (2)

0010-utility.cpp-fix-clang-warnings.patch (884 bytes) - added by mt 3 years ago.
0010-utility.cpp-Add-encoding-decoding-UTF-16LE.patch (2.2 KB) - added by mt 3 years ago.

Download all attachments as: .zip

Change History (11)

comment:1 Changed 3 years ago by mt

Has a Patch: set

comment:2 Changed 3 years ago by jessicah

This is clearly a bug. The spec says we're decoding UTF-16 (rather than UCS-2), so the change to a uint32 is only part of the fix.

See the section about U+10000 and above at https://en.wikipedia.org/wiki/UTF-16. c will need to be OR'd with the next two values.

comment:3 Changed 3 years ago by jessicah

Has a Patch: unset

comment:4 in reply to:  2 Changed 3 years ago by mt

Replying to jessicah:

This is clearly a bug. The spec says we're decoding UTF-16 (rather than UCS-2), so the change to a uint32 is only part of the fix.

Hi, I don't know about UEFI/GPT well, but it seems they use UCS-2, not UTF16 [1][2]. So perhaps we don't need to handle UTF16. [1] http://uefi.blogspot.jp/2009/12/uefi-hii-part-7-character-encoding.html [2] http://lists.alioth.debian.org/pipermail/parted-devel/2015-March/004632.html

comment:5 Changed 3 years ago by jessicah

In that case, the entire if block can be removed, and the put byte helper deleted too I suppose. The todo comment in the other function could be removed too, as implementing that wouldn't be spec compliant either.

comment:6 Changed 3 years ago by jessicah

Hmm, looking at the source code for GPT fdisk, it explicitly supports UTF-16LE rather than UCS-2. The UEFI spec (checked v2.6) on GPT itself doesn't specify use of UCS-2 or UTF-16LE. Wikipedia also specifies UTF-16LE rather than UCS-2. Windows also seems to allow using UTF-16LE encoded volume labels for partitions.

So, I think we should support UTF-16LE here like other systems seem to do. Parted seems to be in the minority. If you don't feel comfortable with implementing, I can add it to my TODO list :)

comment:7 in reply to:  6 Changed 3 years ago by mt

Replying to jessicah:

So, I think we should support UTF-16LE here like other systems seem to do. Parted seems to be in the minority. If you don't feel comfortable with implementing, I can add it to my TODO list :)

Hi, I added UTF-16LE support. Would you review it?

comment:8 Changed 2 years ago by axeld

Owner: changed from axeld to jessicah
Status: newassigned

comment:9 Changed 13 months ago by waddlesplash

Resolution: fixed
Status: assignedclosed

Already merged, according to jessicah.

Note: See TracTickets for help on using tickets.