apache / celix

Apache Celix is a framework for C and C++14 to develop dynamic modular software applications using component and in-process service-oriented programming.
https://celix.apache.org/
Apache License 2.0
158 stars 84 forks source link

Feature/674 add element type to array list #727

Closed pnoltes closed 4 months ago

pnoltes commented 4 months ago

This PR adds support for specific element types with celix_array_list.

With the implementation in this PR users should choice a array element type during creation and when using the existing celix_arrayList_create function a array list with an undefined element type is created. The undefined element type should make the element type introduction backwards compatible.

A few things where I am not certain about / not fully happy about:

Concerning the asserts, my preference is to remove them in the celix_arrayList_get* functions, but keep them in the celix_arrayList_add* functions. I prefer not use the CELIX_ILLEGAL_ARGUMENT return value, because we are then adding the need for runtime error checks on software errors. IMO Ideally the only error return for celix array list functions is ENOMEM.

Note this PR also contains a small fix for push stream so that this compiles with gcc 13.

PengZheng commented 4 months ago

This PR touches several interesting design topics that I want to learn more about. I'm currently reviewing it, which may take a couple of days.

pnoltes commented 4 months ago

Concerning the asserts, my preference is to remove them in the celix_arrayList_get* functions, but keep them in the celix_arrayList_add* functions. I prefer not use the CELIX_ILLEGAL_ARGUMENT return value, because we are then adding the need for runtime error checks on software errors. IMO Ideally the only error return for celix array list functions is ENOMEM.

Another option could be to keep the "undefined element type" array list as-is and introduce specific element typed array lists, e.g.: celix_long_array_list_t, celix_string_array_list_t, etc. The current array list implementation can be reused (with the element type enum private). This would ensure strong typing and prevent wrong usage of array lists.

Note a simple typedef (e.g. typedef struct celix_array_list celix_long_array_list_t;) is not enough and something extra needs to be added to ensure that typed array list are not interchangeable.

PengZheng commented 4 months ago

Note a simple typedef (e.g. typedef struct celix_array_list celix_long_array_list_t;) is not enough and something extra needs to be added to ensure that typed array list are not interchangeable.

This PR is indeed tricky, which involves ownership management and generic container design. Though I'm currently on Chinese New Year's vocation, i.e. busy visiting my relatives following the tradition, I will manage to find time.

pnoltes commented 4 months ago

Note a simple typedef (e.g. typedef struct celix_array_list celix_long_array_list_t;) is not enough and something extra needs to be added to ensure that typed array list are not interchangeable.

This PR is indeed tricky, which involves ownership management and generic container design. Though I'm currently on Chinese New Year's vocation, i.e. busy visiting my relatives following the tradition, I will manage to find time.

For me there is no hurry, so no pressure.

codecov-commenter commented 4 months ago

Codecov Report

Attention: Patch coverage is 96.61654% with 9 lines in your changes are missing coverage. Please review.

Project coverage is 89.66%. Comparing base (6a550a4) to head (9ae479e). Report is 34 commits behind head on master.

Files Patch % Lines
libs/utils/src/array_list.c 96.21% 9 Missing :warning:
Additional details and impacted files ```diff @@ Coverage Diff @@ ## master #727 +/- ## ========================================== + Coverage 88.93% 89.66% +0.73% ========================================== Files 216 217 +1 Lines 24200 26131 +1931 ========================================== + Hits 21522 23431 +1909 - Misses 2678 2700 +22 ```

:umbrella: View full report in Codecov by Sentry.
:loudspeaker: Have feedback on the report? Share it here.

pnoltes commented 4 months ago

@PengZheng : I believe a more effective solution would be to divide the array list (and potentially, in the future, the long/string hash map) into multiple unique array list types based on the element type. The current design of the array list and hash map does not support entries of different types, unlike properties. Therefore, in my opinion, it is more streamlined (albeit requiring additional implementation work) to have distinct array list types for each element type.

I was considering an approach like this:

// Headers (separate header file for each array list type)
typedef struct celix_ptr_array_list celix_ptr_array_list_t; // Allows custom types including a custom copy callback
typedef struct celix_string_array_list celix_string_array_list_t;
typedef struct celix_long_array_list celix_long_array_list_t;
typedef struct celix_double_array_list celix_double_array_list_t;
typedef struct celix_bool_array_list celix_bool_array_list_t;
typedef struct celix_version_array_list celix_version_array_list_t;

void* celix_ptrArrayList_get(celix_ptr_array_list_t* list, int index);
const char* celix_stringArrayList_get(celix_string_array_list_t* list, int index);
long celix_longArrayList_get(celix_long_array_list_t* list, int index);
double celix_doubleArrayList_get(celix_double_array_list_t* list, int index);
bool celix_boolArrayList_get(celix_bool_array_list_t* list, int index);
const celix_version_t* celix_versionArrayList_get(celix_version_array_list_t* list, int index);

// Etc. for set, size, sort...

// C source
typedef struct {
  ....
} celix_generic_array_list_t; //<-- current array list

struct celix_ptr_array_list {
    celix_generic_array_list_t* list;
};

struct celix_string_array_list {
    celix_generic_array_list_t* list;
};

// Etc.

A potential temporary solution to avoid extensive refactoring in the same pull request is to also introduce macros that leverage the C11 _Generic:

#define celix_arrayList_get(ListT, index) _Generic((ListT), \
    celix_ptr_array_list_t*: celix_ptrArrayList_get, \
    celix_string_array_list_t*: celix_stringArrayList_get,   \
)(ListT, index)

// Etc. for set, size, sort...

If we upgrade from -std=gnu99 to -std=c11, we could keep the _Generic approach. However, I'm uncertain if this might be too confusing for users (C has no overloading, but with _Generic it sort-of does).

WDYT?

PengZheng commented 4 months ago

@PengZheng : I believe a more effective solution would be to divide the array list (and potentially, in the future, the long/string hash map) into multiple unique array list types based on the element type. The current design of the array list and hash map does not support entries of different types, unlike properties. Therefore, in my opinion, it is more streamlined (albeit requiring additional implementation work) to have distinct array list types for each element type.

I was considering an approach like this:

// Headers (separate header file for each array list type)
typedef struct celix_ptr_array_list celix_ptr_array_list_t; // Allows custom types including a custom copy callback
typedef struct celix_string_array_list celix_string_array_list_t;
typedef struct celix_long_array_list celix_long_array_list_t;
typedef struct celix_double_array_list celix_double_array_list_t;
typedef struct celix_bool_array_list celix_bool_array_list_t;
typedef struct celix_version_array_list celix_version_array_list_t;

void* celix_ptrArrayList_get(celix_ptr_array_list_t* list, int index);
const char* celix_stringArrayList_get(celix_string_array_list_t* list, int index);
long celix_longArrayList_get(celix_long_array_list_t* list, int index);
double celix_doubleArrayList_get(celix_double_array_list_t* list, int index);
bool celix_boolArrayList_get(celix_bool_array_list_t* list, int index);
const celix_version_t* celix_versionArrayList_get(celix_version_array_list_t* list, int index);

// Etc. for set, size, sort...

// C source
typedef struct {
  ....
} celix_generic_array_list_t; //<-- current array list

struct celix_ptr_array_list {
    celix_generic_array_list_t* list;
};

struct celix_string_array_list {
    celix_generic_array_list_t* list;
};

// Etc.

A potential temporary solution to avoid extensive refactoring in the same pull request is to also introduce macros that leverage the C11 _Generic:

#define celix_arrayList_get(ListT, index) _Generic((ListT), \
    celix_ptr_array_list_t*: celix_ptrArrayList_get, \
    celix_string_array_list_t*: celix_stringArrayList_get,   \
)(ListT, index)

// Etc. for set, size, sort...

If we upgrade from -std=gnu99 to -std=c11, we could keep the _Generic approach. However, I'm uncertain if this might be too confusing for users (C has no overloading, but with _Generic it sort-of does).

WDYT?

The original intention of introducing element type is to hide them from celix_properties so that:

Therefore, run-time polymorphism rather than compile-time polymorphism is needed, right? What will celix_properties look like with proposed design?

pnoltes commented 4 months ago

Therefore, run-time polymorphism rather than compile-time polymorphism is needed, right? What will celix_properties look like with proposed design?

Yes, you are correct. I am struggling a bit with how to (runtime) handling wrong type usage, but indeed creating different array list types for every element type will only make the type aware properties refactoring more complex.

So I agree to keep the current approach and maybe revisit a bit later.