CXX-3072 Reduce Warnings - Code Consistency and Quality Improvements#1178
CXX-3072 Reduce Warnings - Code Consistency and Quality Improvements#1178eramongodb merged 11 commits intomongodb:masterfrom
Conversation
Julia-Garland
left a comment
Julia-Garland
left a comment
There was a problem hiding this comment.
Besides not being sure about your question on write_concern::level::k_acknowledged LGTM!
| } | ||
| break; | ||
|
|
||
| case write_concern::level::k_acknowledged: |
There was a problem hiding this comment.
is the write_concern::level::k_acknowledged do-nothing fallthrough in write_concern.cpp correct?
I checked out the API reference and the MongoDB docs, not sure myself.
There was a problem hiding this comment.
I expect this case is unreachable. If this else block is entered, then nodes() did not return a value. That implies acknowledge_level() cannot be k_acknowledged.
I expect this could assert:
case write_concern::level::k_acknowledged:
// Acknowledged write concern is expected to return `nodes()` above.
MONGOCXX_UNREACHABLE;There was a problem hiding this comment.
Given prior default: behavior, opting to preserve break; behavior rather than introducing an assertion out of caution given the somewhat indirect relationship between is_acknowledged() and get_w(). However, will include a comment documenting the relationship with the returned optional.
kevinAlbs
left a comment
kevinAlbs
left a comment
There was a problem hiding this comment.
The warning reductions are appreciated. LGTM with a possible use of BSONCXX_UNREACHABLE.
| } | ||
| break; | ||
|
|
||
| case write_concern::level::k_acknowledged: |
There was a problem hiding this comment.
I expect this case is unreachable. If this else block is entered, then nodes() did not return a value. That implies acknowledge_level() cannot be k_acknowledged.
I expect this could assert:
case write_concern::level::k_acknowledged:
// Acknowledged write concern is expected to return `nodes()` above.
MONGOCXX_UNREACHABLE;
3 participants
Summary
Resolves CXX-3072. Verified by this patch.
Similar to mongodb/mongo-c-driver#1543 for the C Driver. Addresses 615 Clang warnings (83 unique warnings) with Clang 19.
scoped_bson_value
A long overdue followup to #955:
This addresses
-Wunused-member-functionwarnings inoptions.cppdue to unused functions with internal linkage (default ctor +value_for_init).C-Style Casts
A large part of this PR is devoted to addressing
-Wold-style-castwarnings.Most cases only needed a trivial change from
(T)exprtostatic_cast<T>(expr). Regarding numeric casts, range checks have not been added: those may be audited in the future when an assertion/contract methodology for the C++ Driver is implemented (directly reusing C Driver'sBSON_ASSERT()does not seem ideal).A notable change is the use of
const_castto workaround thevoid*parameter (lack of const) inbsoncxx::v_noabi::types::bson_value::view's private member functions. To avoid unnecessarily breaking ABI compatibility, aconst_castis used instead of updating the parameter type, accompanied by a comment documenting the context and its validity (the const is recovered in the_initfunction + correspondingconvert_from_libbson()overloads updated with the missingconstfor consistency).Miscellaneous
Other warnings addressed by this PR include:
error_code.cppin particular).write_concern::level::k_acknowledgeddo-nothing fallthrough inwrite_concer.cppcorrect?#if defined(X)instead of#if XwhenXis not guaranteed to be defined.static->inlinefor function definitions in headers used by multiple components (where internal linkage does not seem sufficiently motivated).NULL->nullptreven with C APIs (C idioms shouldn't leak into C++ code).0->nullptrin pointer comparisons and initialization.Ignored Warnings
The following warnings were observed but deliberately unaddressed/ignored/silenced:
-Wdocumentation: addressed by CXX-3070 Address -Wdocumentation Clang warnings #1175.-Walign-mismatchinbsoncxx/v_noabi/bsoncxx/private/stack.hh: unclear how to resolve this warning.-Wexit-time-destructorsfor exception types: deferred to a future API major release.-Wglobal-constructors+-Wexit-time-destructorsforuri::k_default_uri: deferred to a future API major release.-Wundefined-func-templateforsoft_declval: deferred to v1 migration PRs (CXX-2745).-Wweak-vtablesfor exception types: deferred to a future API major release.