refactor(core): Use GLib containers in subsystemManager #8
Conversation
This change replaces the use of `icLinkedList` and `icHashMap` with `GList` and `GHashTable`. Refs: BARTON-245 Signed-off-by: gindur772 <Ganesh_Induri@comcast.com>
ganeshInduri434
requested review from
tleacmcsa,
cleithner-comcast,
kfundecmcsa and
mkkoch
as code owners
|
|
| scoped_icLinkedListGeneric *subsystems = subsystemManagerGetRegisteredSubsystems(); | ||
| sbIcLinkedListIterator *subsystemsIt = linkedListIteratorCreate(subsystems); | ||
| while (linkedListIteratorHasNext(subsystemsIt)) | ||
| GList *subsystems = subsystemManagerGetRegisteredSubsystems(); |
There was a problem hiding this comment.
Please use g_auto style cleanup. Especially if refactoring another auto cleanup method (scoped_*)
|
I will write the tests for subSystemManager.c first then will do the refactoring making nothing is broken. |
| scoped_icLinkedListGeneric *subsystems = subsystemManagerGetRegisteredSubsystems(); | ||
| sbIcLinkedListIterator *subsystemsIt = linkedListIteratorCreate(subsystems); | ||
| while (linkedListIteratorHasNext(subsystemsIt)) | ||
| GList *subsystems = subsystemManagerGetRegisteredSubsystems(); |
There was a problem hiding this comment.
Try not to remove scoped bound resource management where it is used. GLib has an expansive g_auto* system for automatic resource cleanup (such as g_autolist).
Another thing is I would recommend exploring swapping lists of strings to GPtrArray as it's going to usually be much more performant than doubly linked lists without the complexity/headache of trying to use arrays of arrays in C directly.
| static bool allDriversStarted = false; | ||
| static subsystemManagerReadyForDevicesFunc readyForDevicesCB = NULL; | ||
| static void checkSubsystemForMigration(SubsystemRegistration *registration); | ||
|
|
||
| static void subsystemRegistrationDestroy(SubsystemRegistration *reg) | ||
| { | ||
| pthread_mutex_destroy(®->mtx); | ||
| free(reg); | ||
| g_free(reg); |
There was a problem hiding this comment.
For the future, no need to swap frees to g_frees. Technically glib's allocator/deallocator implementations will (mostly) use platform-specific malloc/free but we're not too concerned with an extremely high level of portability - free/malloc/calloc are perfectly fine for non-glib types.
|
This looks like it should maybe also be in draft? |
This change replaces the use of
icLinkedListandicHashMapwithGListandGHashTable.Refs: BARTON-245