Skip to content

CXX-3198 Miscellaneous improvements to address feedback in #1306 #1310

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.

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

Merged
merged 3 commits into from
Jan 7, 2025

Conversation

eramongodb
Copy link
Collaborator

Followup to #1306 addressing feedback unrelated to reformatting by ClangFormat.

  • The % UINT8_MAX is unnecessary as signed-to-unsigned integral conversion is well-defined. This addresses the unintentional exclusion of the value 0xff. If use of std::rand() is concerning despite its use as non-trivial stub bytes to satisfy the interfaces being demonstrated as examples, we can consider replacing it with equally arbitrary but non-random non-trivial key material instead (e.g. {0xde, 0xad, 0xbe, 0xef, 0x00, ...}).

  • The call to bson_strncpy is reduced to its simplest form without redundant calls to strlen or std::min to better match its intended usage. (Note: bson_strncpy guarantees null-termination unlike std::strnlen.)

@eramongodb eramongodb requested a review from mdbmes January 6, 2025 16:41
@eramongodb eramongodb self-assigned this Jan 6, 2025
@eramongodb eramongodb requested a review from kevinAlbs January 6, 2025 16:42
Copy link
Contributor

@mdbmes mdbmes left a comment

Choose a reason for hiding this comment

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

Looks good, thanks! I appreciate the use of a placeholder key, letting us omit the misleading stub entirely.

@eramongodb eramongodb merged commit 59a8c10 into mongodb:master Jan 7, 2025
14 of 17 checks passed
@eramongodb eramongodb deleted the cxx-followup branch January 7, 2025 18:59
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

3 participants