-
Notifications
You must be signed in to change notification settings - Fork 456
CDRIVER-5702 deprecate bson-cmp.h
and bson-atomic.h
#1762
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
Conversation
ASSERT is intended for test assertions. BSON_ASSERT is intended for verifying preconditions.
Fuzz test does not have access to private API.
replace use of `bson_in_range_*` with `mcd_in_range_*` replace use of `bson_cmp_*` with `mcd_cmp_*`
To avoid duplicate symbol errors.
Likely not helpful, and may produce extra noise.
Changes were unintended.
And rename `mcd-atomic.c` to `common-atomic.c`
Remove suggested alternatives. Suggested alternatives are now also deprecated.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Thank you for the reviews.
Should these new headers be named
common-*-private.h
Updated the atomic and cmp headers for consistency with (most) other common
headers.
This also applies to
mcd-string.h
andbson-dsl.h
I prefer to do in a follow-up PR to limit scope of this PR.
Latest changes verified with this patch build
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Good catch. Updated.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
One last concern; otherwise, LGTM.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Looks good to me! It's nice to pare down the supported API.
#include <bson-dsl.h> | ||
#include <common-atomic-private.h> |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I'm noticing that bson-dsl and common-atomic-private are sensitive to include order; the 'atomic' module redefines a macro (BSON_IF_GNU_LIKE) that's also used by bson-dsl. I just filed a separate issue about this, https://jira.mongodb.org/browse/CDRIVER-5755
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
The undef
appears to be restored later within bson-atomic.h
/ common-atomic-private.h
:
#ifdef BSON_USE_LEGACY_GCC_ATOMICS
#undef BSON_IF_GNU_LIKE
Is then later restored with:
#ifdef BSON_USE_LEGACY_GCC_ATOMICS
#undef BSON_IF_GNU_LIKE
#define BSON_IF_GNU_LIKE(...) __VA_ARGS__
#endif
Deprecate
bson-atomic.h
andbson-cmp.h
Background & Motivation
These headers are planned for removal in C driver 2.0. The utility APIs are not essential to working with BSON or using the libbson API.
For simplicity, the headers were copied and symbols renamed. No attempt is made to reduce duplication between the deprecated API and non-deprecated API.
Verified with this patch build: https://spruce.mongodb.com/version/6707be7f32bc5700071e890c
Placement of
BSON_GNUC_DEPRECATED
in inline functionsI attempted to add
BSON_GNUC_DEPRECATED
after the inline function parameter list for consistency with other uses ofBSON_GNUC_DEPRECATED
in function declarations:But this resulted in:
Therefore, the
BSON_GNUC_DEPRECATED
is added after the return type: