CDRIVER-4596 Reducing Warnings - MSVC and MinGW Warnings in libbson

[eramongodb]

Description

This PR is a belated followup to #960 that addresses various warnings emitted by MSVC and MinGW on Windows distros, mostly pertaining to signedness mismatch or implicit narrowing conversions of integral types.

Verified by this patch, which adds /WX or -Werror when compiling bson_shared and bson_static with MSVC or MinGW respectively (treat all warnings as errors). These flags are not included in this PR.

The scope of this PR is limited to MSVC and MinGW warnings for the libbson library only (+ some libbson test files and misc. drive-by fixes). Followup PRs will address warnings emitted by MSVC and MinGW for the libmongoc library, example executables, and test executables. When able, warnings emitted by GCC and Clang (far more numerous) will be addressed after that.

Stats

Warning statistics were obtained using the following local toolchain on Windows 10 and Ubuntu 20.04 (within WSL2):

• CMake 3.21.1
• GCC 12.1.0
• Clang 10.0.0
• MSVC 19.29.30133

and the following CMake configuration for GCC/Clang:

• CMAKE_BUILD_TYPE: Debug
• ENABLE_AUTOMATIC_INIT_AND_CLEANUP: OFF
• ENABLE_CLIENT_SIDE_ENCRYPTION: ON
• ENABLE_EXTRA_ALIGNMENT: OFF
• CMAKE_C_FLAGS: -Wall -Wextra -pedantic -Wconversion -Wsign-conversion

and for MSVC:

• CMAKE_BUILD_TYPE: Debug
• ENABLE_AUTOMATIC_INIT_AND_CLEANUP: OFF
• ENABLE_CLIENT_SIDE_ENCRYPTION: ON
• ENABLE_EXTRA_ALIGNMENT: OFF

Relative to 72cba00, this PR reduces the total number of warnings emitted by:

• GCC: 4442 -> 4368 (-1.67%)
• Clang: 4387 -> 4327 (-1.38%)
• MSVC: 265 -> 235 (-1.32%)

and the number of unique warnings (by file, line, type, and message) by:

• GCC: 1348 -> 1313 (-2.60%)
• Clang: 1458 -> 1426 (-2.19%)
• MSVC: 171 -> 150 (-12.28%)

Note: the warning counts do not include D9025 command line option override warnings generated by the kms_message and zlib libraries.

Methodology

C4018 Signedness Mismatch Warnings

Most signedness mismatch warnings are addressed by the use of bson_cmp_* utilities instead of simple comparison operators, e.g.:

bool before(int32_t a, uint32_t b) {
  return a < b;
}

bool after(int32_t a, uint32_t b) {
  return bson_cmp_less_su(a, b);
}

Otherwise, either the type of relevant variables, parameters, or data members were adjusted to avoid signedness inconsistencies, or an assertion of bson_in_range_* was added prior to an explicit cast to the target type, e.g.:

void usage(uint32_t param);

void before(int32_t value) {
  usage(value);
}

void after(int32_t value) {
  BSON_ASSERT(bson_in_range_signed(uint32_t, value));
  usage((uint32_t)value);
}

C4028 Parameter Inconsistency Warnings

Several MSVC versions (pessimistically) warn when the type of a parameter in a function declaration differs from the definition, even when the difference is due to top-level const-qualifiers. The declarations were updated to match the definitions.

C4146 Unsigned Unary Negation Warnings

Newer MSVC versions warn when the unary negation operator is used on an unsigned type, even if it has well-defined behavior. Added suppression of C4146 where appropriate.

C4244 Narrowing Conversion Warnings

Most of these warnings are addressed by the addition of a range-check assertion as described above in C4018. For functions that return an error code or error messages, an appropriate error and/or return value is used instead of an assertion. If range checks already exist, only the explicit cast is added.

C4267 size_t Conversion Warnings

MSVC has a separate warning type for size_t conversions than for other narrowing conversion warnings, but they are effectively the same as C4244 narrowing conversion warnings.

C4756 Constant Arithmetic Overflow Warnings

VC 2013's definition of the INFINITY macro triggers overflow warnings despite being intentional behavior. Furthermore, VS 2013 does not appear to support inline warning suppression with #pragma warning(suppress : ...) or #pragma warning(disable : ...). Therefore, the /wd4756 compile option was added to bson_shared and bson_static when compiling with VS 2013 or older to disable the warning entirely, in favor of depending on other toolchain combinations to detect undesirable overflow conditions.

Miscellaneous Warnings

As drive-by fixes and improvements not related to the above:

• replaced (void)expr; with BSON_MAYBE_UNUSED int ret = expr; in _aligned_alloc_impl to address GCC unused result warnings that were not being silenced by (void). See https://gcc.gnu.org/bugzilla/show_bug.cgi?id=66425 for context.

• addressed a scan-build null pointer argument warning in _mongoc_getenv due to repeated calls to getenv defeating the static analyzer's ability to recognize the null pointer check.

[eramongodb]

Latest changes verified by this patch.

[kevinAlbs]

LGTM. The efforts to reduce warnings are appreciated.

[kevinAlbs]

Suggested change

	const uint64_t a_count = ((bson_type_metrics_t *) b)->count;
	const uint64_t b_count = ((bson_type_metrics_t *) a)->count;
	const uint64_t a_count = ((bson_type_metrics_t *) a)->count;
	const uint64_t b_count = ((bson_type_metrics_t *) b)->count;

[vector-of-bool]

LGTM