cppcheck: struct iocontrol_str v1 and v2 named equally in source tree

[smoe]

Cppcheck made some noise in emc/iotask and I tend to agree that something could be improved here.

$ cppcheck iotask/*.cc
Checking iotask/ioControl.cc ...
Checking iotask/ioControl.cc: TOOL_NML...
1/2 files checked 45% done
Checking iotask/ioControl_v2.cc ...
Checking iotask/ioControl_v2.cc: TOOL_NML...
2/2 files checked 100% done
iotask/ioControl.cc:90:1: error: The one definition rule is violated, different classes/structs have the same name 'iocontrol_str' [ctuOneDefinitionRuleViolation]
struct iocontrol_str {
^
iotask/ioControl_v2.cc:140:1: note: The one definition rule is violated, different classes/structs have the same name 'iocontrol_str'
struct iocontrol_str {
^
iotask/ioControl.cc:90:1: note: The one definition rule is violated, different classes/structs have the same name 'iocontrol_str'
struct iocontrol_str {

I then looked at these (big) struct definitions, exported them into files a and b and after some formatting found that the only difference indeed v2 is just an extension of the prior:

>     // v2 protocol
>     hal_bit_t *emc_abort;
>     hal_bit_t *emc_abort_ack;
>     hal_s32_t *emc_reason;
>     hal_bit_t *toolchanger_fault;
>     hal_bit_t *toolchanger_fault_ack;
>     hal_s32_t *toolchanger_reason;
>     hal_bit_t *start_change;
>     hal_bit_t *start_change_ack;
>     hal_bit_t *toolchanger_faulted;
>     hal_bit_t *toolchanger_clear_fault;
>     hal_s32_t *state;

Defined in a .cc file, there is only a single variable iocontrol_data defined as a pointer to it. Both that (global) variable and the struct name are kept the same. Declaring the variables as static has not helped cppcheck (but it should have, right?).

I admit not to have the ultimate answers here. My immediate thought was to present an iocontrol typedef that is a union of the two versions, but I was never a fan of unions. Also, should a stream of iocontrols ever be protocolled into a file, it would likely be handy to have the data type available in a separate include file.

My immediate shot was to just rename the earlier definition.

diff --git a/src/emc/iotask/ioControl.cc b/src/emc/iotask/ioControl.cc
index f97d5f9243..8e0db70ded 100644
--- a/src/emc/iotask/ioControl.cc
+++ b/src/emc/iotask/ioControl.cc
@@ -87,7 +87,7 @@ static int      random_toolchanger  = 0;
 static tooldb_t io_db_mode          = DB_NOTUSED;
 static char     db_program[LINELEN] = {0};

-struct iocontrol_str {
+struct iocontrol_str_v1 {
     hal_bit_t *user_enable_out;        /* output, TRUE when EMC wants stop */
     hal_bit_t *emc_enable_in;        /* input, TRUE on any external stop */
     hal_bit_t *user_request_enable;        /* output, used to reset ENABLE latch */
@@ -295,7 +295,7 @@ static int iocontrol_hal_init(void)
     }

     /* STEP 2: allocate shared memory for iocontrol data */
-    iocontrol_data = (iocontrol_str *) hal_malloc(sizeof(iocontrol_str));
+    iocontrol_data = (iocontrol_str_v1 *) hal_malloc(sizeof(iocontrol_str_v1));
     if (iocontrol_data == 0) {
         rtapi_print_msg(RTAPI_MSG_ERR,
                         "IOCONTROL: ERROR: hal_malloc() failed\n");

but I may be missing something?!?

Many thanks Steffen

[rene-dev]

fixed by https://github.com/LinuxCNC/linuxcnc/pull/2497