[SERVER-69612] Audit the codebase for signed use of char types Created: 13/Sep/22  Updated: 27/Oct/23  Resolved: 15/Sep/22

Status: Closed
Project: Core Server
Component/s: None
Affects Version/s: None
Fix Version/s: None

Type: Task Priority: Major - P3
Reporter: Josef Ahmad Assignee: Jordi Olivares Provencio
Resolution: Works as Designed Votes: 0
Labels: None
Remaining Estimate: Not Specified
Time Spent: Not Specified
Original Estimate: Not Specified

Issue Links:
Related
related to WT-9857 Audit code: signed-ness of char type ... Closed
related to SERVER-69784 Fix instances of signed char misuse Closed
is related to SERVER-69728 Remove BufBuilder::appendChar in favo... Closed
is related to SERVER-69729 Enable clang-tidy check for signed ch... Closed
Sprint: Execution Team 2022-09-19
Participants:

 Description   

Signedness of "char" is platform-specific, e.g. it is signed on x86-linux, it is typically unsigned on ARM. For portability we should stick to using an unsigned type.



 Comments   
Comment by Jordi Olivares Provencio [ 15/Sep/22 ]

In cases where char is used to contain a number it is always below 128, so uses of it are correct. The other uses for a char are to treat it as a byte (uses of which could be replaced by std::byte).

geert.bosch@mongodb.com I haven't found a problem other than potential confusion by casting the char as a byte so I'll close the ticket.

I've opened SERVER-69729 and SERVER-69728 to help avoid potential errors in the future.

Comment by Jordi Olivares Provencio [ 15/Sep/22 ]

Looking at other branches with the same patterns leads to the same conclusion: Only the Enum handling worries me, but it seems to be correctly used as they are interpreted as a single raw byte when used.

Something that could be improved is the single byte handling of the BSON BufBuilder. There's various methods used for handling a char but conceptually we're using them to append a single byte. Refactoring the code so that the 3 char methods are replaced by a method called appendByte(std::byte) would help clarify the intent.

Comment by Jordi Olivares Provencio [ 14/Sep/22 ]

Searching for the following:

  • static_cast<char>
  • (char) casting

Points largely to the BSONType and BSONBinData enum handling. In particular two key values can be affected by the signedness of char: BSONType::MinKey and BSONBinData::bdtCustom as they are -1 and 128 respectively.

The latter gets used in GeoHash here and here.

Comment by Gregory Noma [ 13/Sep/22 ]

std::byte might be useful

Generated at Thu Feb 08 06:13:55 UTC 2024 using Jira 9.7.1#970001-sha1:2222b88b221c4928ef0de3161136cc90c8356a66.