[CDRIVER-6043] Modify definitions in `intutil` to avoid compound-literals #2056

vector-of-bool · 2025-07-09T17:26:39Z

The following changes are made:

We avoid compound literals expanded from macros. This is to avoid buggy compound-literal support in VS 2017 (See CDRIVER-6043).

Instead, we pass the relevant argument to a function that initializes the new aggregate object.

This has the downside that we cannot use the macros in constant expressions, but that was a minor feature that wasn't really used anywhere.
Add a new macro to define bit masks. This simplifies the expressions that build bit patterns used for two's complement arithmetic.
Rename the members of mlib_upsized_integer to give them meaningful names.

This doesn't fully fix CDRIVER-6043, but will avoid future subtle issues with VS 2017.

The followin changes are made: - We avoid compound literals expanded from macros. This is to avoid buggy compound-literal support in VS 2017 (See CDRIVER-6043). Instead, we pass the relevant argument to a function that initializes the new aggregate object. This has the downside that we cannot use the macros in constant expressions, but that was a minor feature that wasn't really used anywhere. - Add a new macro to define bit masks. This simplifies the expressions that build bit patterns used for two's complement arithmetic. - Rename the members of `mlib_upsized_integer` to give them meaningful names.

eramongodb

Very welcome changes! Minor suggestions mostly concerning type/signedness consistency, but otherwise LGTM.

eramongodb · 2025-07-09T19:29:30Z

src/common/src/mlib/intutil.h

+/**
+ * @brief Like `sizeof`, but returns the number of bits in the object representation
+ */
+#define mlib_bitsizeof(T) ((sizeof (T)) * CHAR_BIT)


Suggested change

#define mlib_bitsizeof(T) ((sizeof (T)) * CHAR_BIT)

#define mlib_bitsizeof(T) ((sizeof (T)) * ((size_t) CHAR_BIT))

Type consistency.

eramongodb · 2025-07-09T19:36:21Z

src/common/src/mlib/intutil.h

+/**
+ * @brief Generate a mask of contiguous bits.
+ *
+ * @param NumOnes The number of contiguous 1 bits


Suggested change

* @param NumOnes The number of contiguous 1 bits

* @param NumOnes The positive number of contiguous 1 bits

To document the requirement that NumOnes > 0:

// warning: shift count >= width of type [-Wshift-count-overflow] // runtime error: shift exponent 64 is too large for 64-bit type 'unsigned long' mlib_bits(0, 0)

Tweaked the definition to support setting zero 1 bits to "do the right thing", also tests for this.

eramongodb · 2025-07-09T19:39:15Z

src/common/src/mlib/intutil.h

+ * @brief Generate a mask of contiguous bits.
+ *
+ * @param NumOnes The number of contiguous 1 bits
+ * @param NumZeros The number of contiguous 0 bits to set in the low position


Suggested change

* @param NumZeros The number of contiguous 0 bits to set in the low position

* @param NumZeros The non-negative number of contiguous 0 bits to set in the low position

To document the requirement that NumZeroes >= 0.

eramongodb · 2025-07-09T19:44:47Z

src/common/src/mlib/intutil.h

+#define mlib_bits(NumOnes, NumZeros) ( \
+      /* Create a set of N 1 bits */ \
+      (/* Create a mask of all 0b1111... */ \
+         ~UINTMAX_C(0) \
+         /* Shift down the chop off to only the number of 1 bits we want */ \
+         >> (mlib_bitsizeof(uintmax_t) - (uintmax_t)(NumOnes))) \
+      /* Shift left to add the low zeros */\
+      << ((uintmax_t)(NumZeros)) \
+   )


Suggested change

#define mlib_bits(NumOnes, NumZeros) ( \

/* Create a set of N 1 bits */ \

(/* Create a mask of all 0b1111... */ \

~UINTMAX_C(0) \

/* Shift down the chop off to only the number of 1 bits we want */ \

>> (mlib_bitsizeof(uintmax_t) - (uintmax_t)(NumOnes))) \

/* Shift left to add the low zeros */\

<< ((uintmax_t)(NumZeros)) \

)

#define mlib_bits(NumOnes, NumZeros) ( \

(~UINTMAX_C(0) >> (mlib_bitsizeof(uintmax_t) - (uintmax_t)(NumOnes))) \

<< ((uintmax_t)(NumZeros)))

I think the thorough inline comments may be hurting readability more than helping. Suggest deferring the explanation to the doc comment above, which imo is sufficient.

eramongodb · 2025-07-09T19:46:59Z

src/common/src/mlib/intutil.h

+        ? ((T) mlib_bits(mlib_bitsizeof(T) - 1, 0)) \
+        : ((T) mlib_bits(mlib_bitsizeof(T),     0))))


Suggested change

? ((T) mlib_bits(mlib_bitsizeof(T) - 1, 0)) \

: ((T) mlib_bits(mlib_bitsizeof(T), 0))))

? ((T) mlib_bits(mlib_bitsizeof(T) - 1u, 0)) \

: ((T) mlib_bits(mlib_bitsizeof(T), 0))))

Signedness consistency.

eramongodb · 2025-07-09T20:03:02Z

src/common/src/mlib/ckdint.h

@@ -460,7 +458,7 @@ static inline bool (mlib_mul) (uintmax_t *dst, bool dst_signed, bool a_signed, u
   mlib_noexcept
 {
   // Multiplication is a lot more subtle
-   const uintmax_t signbit = (UINTMAX_C (1) << ((sizeof (intmax_t) * CHAR_BIT) - 1));
+   const uintmax_t signbit = mlib_bits (1, mlib_bitsizeof (uintmax_t) - 1);


Suggested change

const uintmax_t signbit = mlib_bits (1, mlib_bitsizeof (uintmax_t) - 1);

const uintmax_t signbit = mlib_bits (1, mlib_bitsizeof (uintmax_t) - 1u);

eramongodb · 2025-07-09T20:03:24Z

src/common/src/mlib/ckdint.h

@@ -359,7 +357,7 @@ static inline bool (mlib_sub) (uintmax_t *dst, bool dst_signed, bool a_signed, u
 {
   // Perform the subtraction using regular wrapping arithmetic
   const uintmax_t diff = *dst = a - b;
-   const uintmax_t signbit = (UINTMAX_C (1) << ((sizeof (intmax_t) * CHAR_BIT) - 1));
+   const uintmax_t signbit = mlib_bits (1, mlib_bitsizeof (uintmax_t) - 1);


Suggested change

const uintmax_t signbit = mlib_bits (1, mlib_bitsizeof (uintmax_t) - 1);

const uintmax_t signbit = mlib_bits (1, mlib_bitsizeof (uintmax_t) - 1u);

eramongodb · 2025-07-09T20:03:29Z

src/common/src/mlib/ckdint.h

@@ -265,7 +263,7 @@ static inline bool (mlib_add) (uintmax_t *dst, bool dst_signed, bool a_signed, u
   // Perform regular wrapping arithmetic on the unsigned value. The bit pattern
   // is equivalent if there is two's complement signed arithmetic.
   const uintmax_t sum = *dst = a + b;
-   const uintmax_t signbit = (UINTMAX_C (1) << ((sizeof (intmax_t) * CHAR_BIT) - 1));
+   const uintmax_t signbit = mlib_bits (1, mlib_bitsizeof (uintmax_t) - 1);


Suggested change

const uintmax_t signbit = mlib_bits (1, mlib_bitsizeof (uintmax_t) - 1);

const uintmax_t signbit = mlib_bits (1, mlib_bitsizeof (uintmax_t) - 1u);

eramongodb · 2025-07-09T20:04:50Z

src/common/src/mlib/ckdint.h

-#define _mlibMinofSigned(V) ((0 & (V)) - (UINTMAX_C (1) << ((sizeof (V) * CHAR_BIT) - 1)))
+#define _mlibMinofSigned(V) \
+   /* NOLINTNEXTLINE(bugprone-sizeof-expression) */ \
+   (0 - mlib_bits(1, mlib_bitsizeof(V) - 1))


Suggested change

(0 - mlib_bits(1, mlib_bitsizeof(V) - 1))

(0 - mlib_bits(1, mlib_bitsizeof(V) - 1u))

eramongodb · 2025-07-09T20:04:56Z

src/common/src/mlib/ckdint.h

-#define _mlibMaxofSigned(V) (_mlibMaxofUnsigned (V) >> 1ull)
+#define _mlibMaxofSigned(V) \
+   /* NOLINTNEXTLINE(bugprone-sizeof-expression) */ \
+   mlib_bits(mlib_bitsizeof(V) - 1, 0)


Suggested change

mlib_bits(mlib_bitsizeof(V) - 1, 0)

mlib_bits(mlib_bitsizeof(V) - 1u, 0)

vector-of-bool requested a review from eramongodb July 9, 2025 17:26

vector-of-bool requested a review from a team as a code owner July 9, 2025 17:26

eramongodb approved these changes Jul 9, 2025

View reviewed changes

vector-of-bool added 5 commits July 9, 2025 15:50

Use unsigned literals in bit manip

c972e71

Tweak mlib_bits to support zero set bits

fe5c0be

Change static tests to runtime tests

9ae2d1a

Merge branch 'master' into CDRIVER-6043-avoid-compound-lit-in-macros

3aedda7

Post-merge spelling changes

c5b7f65

vector-of-bool merged commit afb406d into mongodb:master Jul 11, 2025
40 of 42 checks passed

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Uh oh!

[CDRIVER-6043] Modify definitions in `intutil` to avoid compound-literals #2056

[CDRIVER-6043] Modify definitions in `intutil` to avoid compound-literals #2056

Uh oh!

vector-of-bool commented Jul 9, 2025 •

edited

Loading

Uh oh!

eramongodb left a comment

Uh oh!

eramongodb Jul 9, 2025

Uh oh!

eramongodb Jul 9, 2025

Uh oh!

vector-of-bool Jul 9, 2025

Uh oh!

eramongodb Jul 9, 2025

Uh oh!

eramongodb Jul 9, 2025

Uh oh!

eramongodb Jul 9, 2025

Uh oh!

eramongodb Jul 9, 2025

Uh oh!

eramongodb Jul 9, 2025

Uh oh!

eramongodb Jul 9, 2025

Uh oh!

eramongodb Jul 9, 2025

Uh oh!

eramongodb Jul 9, 2025

Uh oh!

Uh oh!

Uh oh!

	#define mlib_bitsizeof(T) ((sizeof (T)) * CHAR_BIT)
	#define mlib_bitsizeof(T) ((sizeof (T)) * ((size_t) CHAR_BIT))

	* @param NumOnes The number of contiguous 1 bits
	* @param NumOnes The positive number of contiguous 1 bits

	* @param NumZeros The number of contiguous 0 bits to set in the low position
	* @param NumZeros The non-negative number of contiguous 0 bits to set in the low position

		? ((T) mlib_bits(mlib_bitsizeof(T) - 1, 0)) \
		: ((T) mlib_bits(mlib_bitsizeof(T), 0))))

	const uintmax_t signbit = mlib_bits (1, mlib_bitsizeof (uintmax_t) - 1);
	const uintmax_t signbit = mlib_bits (1, mlib_bitsizeof (uintmax_t) - 1u);

	(0 - mlib_bits(1, mlib_bitsizeof(V) - 1))
	(0 - mlib_bits(1, mlib_bitsizeof(V) - 1u))

	mlib_bits(mlib_bitsizeof(V) - 1, 0)
	mlib_bits(mlib_bitsizeof(V) - 1u, 0)

[CDRIVER-6043] Modify definitions in intutil to avoid compound-literals #2056

[CDRIVER-6043] Modify definitions in intutil to avoid compound-literals #2056

Uh oh!

Conversation

vector-of-bool commented Jul 9, 2025 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

eramongodb left a comment

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Uh oh!

Uh oh!

[CDRIVER-6043] Modify definitions in `intutil` to avoid compound-literals #2056

[CDRIVER-6043] Modify definitions in `intutil` to avoid compound-literals #2056

vector-of-bool commented Jul 9, 2025 •

edited

Loading