Draft: meson: increase compiler checks by vjardin · Pull Request #7 · vjardin/bmr

vjardin · 2025-11-26T01:38:08Z

Use:
'warning_level=everything',
'werror=true',

Use: 'warning_level=everything', 'werror=true',

TODO: set Werror only for runners on github.

gcc provides good tips on pure C functions, let's ack't them. pure: functions that are idempotent since they do not rely on internal states. They are like some "const", weaker. They do not have side effects. use case: x = le16(p); y = le16(p); could be optimized to: x = le16(p); # le16 is pure y = x;

Copilot

Pull request overview

This PR increases the compiler warning level from 3 to "everything" in the Meson build configuration, addressing warnings that arise from stricter compiler checks. The PR is marked as draft with werror=true commented out, indicating that not all warnings may be resolved yet.

Introduces a D() macro for explicit double type conversions to satisfy type-checking warnings
Adds self-header includes to all implementation files to ensure header self-containment
Converts type declarations from int to more appropriate types (size_t, uint8_t) for improved type safety

Reviewed changes

Copilot reviewed 33 out of 33 changed files in this pull request and generated 9 comments.

Show a summary per file

File	Description
meson.build	Changes warning level from 3 to "everything"; werror commented out (draft)
src/pmbus_io.h	Adds D() macro for double casts, BMR_PURE attribute, changes pmbus_wr_block len param to uint8_t
src/pmbus_io.c	Updates le16/le32 formatting, adds BMR_PURE attributes, updates pmbus_wr_block signature
src/util_lin.h	Updates u16_round_sat_pos with D() macro, changes literals to UINT16_MAX, includes pmbus_io.h
src/util_json.c	Changes add_hex_ascii parameter from int to size_t, adds proper casts
src/temp_cmd.c	Rewrites sign_extend function to avoid UB, adds epsilon-based zero comparison, adds assert.h
src/timing_cmd.c	Adds explicit padding field (__pad1) to timing_profile struct
src/read_cmd.c	Adds default case with __builtin_unreachable() to switch statement
src/operation_cmd.c	Adds default case with error handling to switch statement
src/fault_cmd.c	Adds default case returning error string to resp_mode_name function
src/mfr_user_data.c	Changes loop variable and length tracking from int to size_t, adds length validation
src/mfr_hrr.c	Introduces FLAG_SET8/CLEAR8/ISSET8 macros for type-safe flag operations
src/mfr_fwdata.c	Changes loop variables from int to size_t
src/main.c	Changes opt_addr from int to uint8_t
src/vout_cmd.c	Updates double literals using D() macro
src/vin_cmd.c	Adds self-header include
src/write_protect_cmd.c	Adds self-header include
src/status_cmd.c	Adds self-header include
src/salert_cmd.c	Adds self-header include
src/rw_cmd.c	Adds self-header include
src/pgood_cmd.c	Adds self-header include
src/onoff_cmd.c	Adds self-header include
src/mfr_status_data.c	Adds self-header include
src/mfr_snapshot.c	Adds self-header include
src/mfr_save_restore.c	Adds self-header include
src/mfr_restart.c	Adds self-header include
src/mfr_ramp_data.c	Adds self-header include
src/mfr_multipin.c	Adds self-header include
src/mfr_id.c	Adds self-header include
src/mfr_addr_offset.c	Adds self-header include
src/interleave_cmd.c	Adds self-header include
src/freq_cmd.c	Adds self-header include
src/capability_cmd.c	Adds self-header include

💡 Add Copilot custom instructions for smarter, more guided reviews. Learn how to get started.

Copilot · 2025-11-26T08:10:06Z

src/temp_cmd.c

+  if (v == D(0.0f))
    return 0;
+#else
+  /* epsilon: smallest assume it is same than 0 */


Typo in comment: "smallest assume it is same than 0" should be "smallest value assumed to be the same as 0" or similar.

Suggested change

/* epsilon: smallest assume it is same than 0 */

/* epsilon: values smaller than this are assumed to be zero */

Copilot · 2025-11-26T08:10:07Z

src/temp_cmd.c

    return 0;
+#else
+  /* epsilon: smallest assume it is same than 0 */
+  const double lin11_eps = D(1.0f / (float)(1u << 16));


[nitpick] The expression D(1.0f / (float)(1u << 16)) performs the division in float precision before casting to double. This can lead to precision loss. Consider using 1.0 / (double)(1u << 16) or D(1.0) / D(65536.0) to ensure the calculation is done in double precision.

Suggested change

const double lin11_eps = D(1.0f / (float)(1u << 16));

const double lin11_eps = D(1.0) / D(65536.0);

Copilot · 2025-11-26T08:10:07Z

src/pmbus_io.h

@@ -5,6 +5,15 @@
 #include <stdint.h>
 #include <stdbool.h>



[nitpick] The D() macro casts float literals to double, but using float literals (e.g., 0.0f) that are then cast to double is unnecessary and may reduce precision. Consider using double literals directly (e.g., 0.0 instead of D(0.0f)) to avoid the intermediate float conversion and potential precision loss.

Suggested change

/* Use double literals (e.g., 0.0), not float literals (e.g., 0.0f), with D() to avoid unnecessary float-to-double conversion and potential precision loss. */

Copilot · 2025-11-26T08:10:07Z

src/mfr_hrr.c

-    json_object_set_new(o, "dls_mode", json_string(((uint8_t) rb & BIT_DLS) ? "nonlinear" : "linear"));
-    json_object_set_new(o, "artdlc_enabled", json_boolean(!!((uint8_t) rb & BIT_ARTDLC)));
-    json_object_set_new(o, "dbv_enabled", json_boolean(!!((uint8_t) rb & BIT_DBV)));
+    json_object_set_new(o, "require_pec", json_boolean(FLAG_ISSET8(rb,BIT_PEC)));


Missing space after FLAG_ISSET8(rb, - there should be a space after the comma for consistency with the other calls on lines 225-228.

Suggested change

json_object_set_new(o, "require_pec", json_boolean(FLAG_ISSET8(rb,BIT_PEC)));

json_object_set_new(o, "require_pec", json_boolean(FLAG_ISSET8(rb, BIT_PEC)));

Copilot · 2025-11-26T08:10:08Z

src/util_lin.h

 u16_round_sat_pos(double x) {
-  if (isnan(x))
-    return 0;
+  if ((x <= D(0.0f)) || isnan(x))


The condition check (x <= D(0.0f)) now rejects zero values, but the original code only checked if x was NaN. This changes the function behavior - zero is a valid positive value that should return 0, not be rejected. The condition should be (x < D(0.0f)) (strictly less than) rather than <= to maintain the original behavior for zero.

Suggested change

if ((x <= D(0.0f)) || isnan(x))

if ((x < D(0.0f)) || isnan(x))

Copilot · 2025-11-26T08:10:08Z

src/mfr_user_data.c


-    if (pmbus_wr_block(fd, MFR_USER_DATA_00, b, n) < 0) {
+    if (n > 255) {
+      fprintf(stderr, "USER_DATA_00 write: len %zu > 255", n);


Missing newline character at end of error message. The error message should end with \n for consistent formatting: fprintf(stderr, "USER_DATA_00 write: len %zu > 255\n", n);

Suggested change

fprintf(stderr, "USER_DATA_00 write: len %zu > 255", n);

fprintf(stderr, "USER_DATA_00 write: len %zu > 255\n", n);

Copilot · 2025-11-26T08:10:09Z

src/pmbus_io.c

+  return (uint16_t)(
+        (uint16_t)p[0]
+      | ((uint16_t)p[1] << 8)
+      );
+


[nitpick] Inconsistent formatting: the closing parenthesis and semicolon are on a separate line with extra blank line, while le32 below maintains the traditional formatting. Consider keeping the formatting consistent between these two functions.

Suggested change

return (uint16_t)(

(uint16_t)p[0]

| ((uint16_t)p[1] << 8)

);

return (uint16_t) p[0]

| ((uint16_t) p[1] << 8)

;

Copilot · 2025-11-26T08:10:09Z

src/util_json.c

  json_object_set_new(dst, "ascii", json_stringn((const char *) buf, n));

-  char *hex = (char *) malloc((size_t) n * 2 + 1);
+  char *hex = (char *) malloc(n * 2 + 1);


Missing NULL check after malloc. If malloc fails and returns NULL, the subsequent sprintf call on line 17 will cause a segmentation fault. Add a NULL check: if (!hex) return; or similar error handling.

Suggested change

char *hex = (char *) malloc(n * 2 + 1);

char *hex = (char *) malloc(n * 2 + 1);

if (!hex)

return;

Copilot · 2025-11-26T08:10:09Z

src/operation_cmd.c

        break;
+      default:
+        fprintf(stderr, "invalid pmbus operation\n");
+	return 2;


Tab character used for indentation instead of spaces. This is inconsistent with the surrounding code which uses spaces.

Suggested change

return 2;

return 2;

thbtcllt

Set warning_level to everything is a little bit excessive.
Fix some warnings will lead to strange code especially when MACRO are used.
For example to fix the clang warning:
STATUS_TEMPERATURE_FIELDS(EMIT_STATUS_BYTE);
it is necessary to remove the ';' that will be very ugly when we read the code.

meson: increase compiler checks

b8361b1

Use: 'warning_level=everything', 'werror=true',

vjardin requested review from Copilot and thbtcllt and removed request for Copilot November 26, 2025 01:38

Copilot started reviewing on behalf of vjardin November 26, 2025 01:38 View session

vjardin changed the title ~~meson: increase compiler checks~~ Draft: meson: increase compiler checks Nov 26, 2025

Copilot finished reviewing on behalf of vjardin November 26, 2025 01:41

vjardin added 2 commits November 26, 2025 08:57

WIP - do not merge - drop Werror

b45d1ed

TODO: set Werror only for runners on github.

Copilot AI review requested due to automatic review settings November 26, 2025 08:05

Copilot started reviewing on behalf of vjardin November 26, 2025 08:05 View session

Copilot finished reviewing on behalf of vjardin November 26, 2025 08:08

Copilot AI reviewed Nov 26, 2025

View reviewed changes

thbtcllt reviewed Nov 26, 2025

View reviewed changes

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Draft: meson: increase compiler checks#7

Draft: meson: increase compiler checks#7
vjardin wants to merge 3 commits intomainfrom
vj_fixcompile

vjardin commented Nov 26, 2025

Uh oh!

Copilot AI left a comment

Uh oh!

Copilot AI Nov 26, 2025

Uh oh!

Copilot AI Nov 26, 2025

Uh oh!

Copilot AI Nov 26, 2025

Uh oh!

Copilot AI Nov 26, 2025

Uh oh!

Copilot AI Nov 26, 2025

Uh oh!

Copilot AI Nov 26, 2025

Uh oh!

Copilot AI Nov 26, 2025

Uh oh!

Copilot AI Nov 26, 2025

Uh oh!

Copilot AI Nov 26, 2025

Uh oh!

thbtcllt left a comment

Uh oh!

Reviewers

Assignees

Labels

Projects

Milestone

Development

Uh oh!

3 participants

	/* epsilon: smallest assume it is same than 0 */
	/* epsilon: values smaller than this are assumed to be zero */

	const double lin11_eps = D(1.0f / (float)(1u << 16));
	const double lin11_eps = D(1.0) / D(65536.0);



	/* Use double literals (e.g., 0.0), not float literals (e.g., 0.0f), with D() to avoid unnecessary float-to-double conversion and potential precision loss. */

	json_object_set_new(o, "require_pec", json_boolean(FLAG_ISSET8(rb,BIT_PEC)));
	json_object_set_new(o, "require_pec", json_boolean(FLAG_ISSET8(rb, BIT_PEC)));

	if ((x <= D(0.0f)) \|\| isnan(x))
	if ((x < D(0.0f)) \|\| isnan(x))

	fprintf(stderr, "USER_DATA_00 write: len %zu > 255", n);
	fprintf(stderr, "USER_DATA_00 write: len %zu > 255\n", n);

Conversation

vjardin commented Nov 26, 2025

Uh oh!

Copilot AI left a comment

Choose a reason for hiding this comment

Pull request overview

Reviewed changes

Uh oh!

Copilot AI Nov 26, 2025

Choose a reason for hiding this comment

Uh oh!

Copilot AI Nov 26, 2025

Choose a reason for hiding this comment

Uh oh!

Copilot AI Nov 26, 2025

Choose a reason for hiding this comment

Uh oh!

Copilot AI Nov 26, 2025

Choose a reason for hiding this comment

Uh oh!

Copilot AI Nov 26, 2025

Choose a reason for hiding this comment

Uh oh!

Copilot AI Nov 26, 2025

Choose a reason for hiding this comment

Uh oh!

Copilot AI Nov 26, 2025

Choose a reason for hiding this comment

Uh oh!

Copilot AI Nov 26, 2025

Choose a reason for hiding this comment

Uh oh!

Copilot AI Nov 26, 2025

Choose a reason for hiding this comment

Uh oh!

thbtcllt left a comment

Choose a reason for hiding this comment

Uh oh!

Reviewers

Assignees

Labels

Projects

Milestone

Development

Uh oh!

3 participants