UI Code Quality: Use LISTBASE_FOREACH in interface directory
ClosedPublic
Actions

Authored by Hans Goudey (HooglyBoogly) on Aug 16 2020, 4:02 PM.

Details

Reviewers

Julian Eisel (Severin)
Campbell Barton (campbellbarton)

Commits

rB349eebd7d145: UI Code Quality: Use LISTBASE_FOREACH in interface directory

Summary

I used the LISTBASE_FOREACH, LISTBASE_FOREACH_BACKWARDS, and LISTBASE_FOREACH_MUTABLE
macros instead of almost all of the remaining for loops in the interface directory.

I only skipped a few loops in the monstrously nightmarish ui_handle_menu_event function.

Also, I only changed variable names where I had to to prevent redeclarations.

Diff Detail

Repository: rB Blender

Event Timeline

Hans Goudey (HooglyBoogly) requested review of this revision.Aug 16 2020, 4:02 PM

Hans Goudey (HooglyBoogly) created this revision.

Harbormaster completed remote builds in B9547: Diff 27765.Aug 16 2020, 4:02 PM

Hans Goudey (HooglyBoogly) edited the summary of this revision. (Show Details)Aug 16 2020, 4:18 PM

Hans Goudey (HooglyBoogly) added a project: User Interface.

Generally looks fine, there is one issue with incrementing being moved out of for loops which I'd rather avoid, details inline.

source/blender/editors/interface/interface_layout.c
4429–4443	Using the `for` loop for the counters is less error prone, as using `continue` here will skip the counter (while not an issue in this case, I don't think it's a good convention). We could have `LISTBASE_FOREACH_INDEX` macro for this, as we have for BMesh iterators.
source/blender/editors/interface/interface_templates.c
7363–7364	Again, counter being removed from for loop.

This revision now requires changes to proceed.Aug 17 2020, 12:16 PM

Campbell Barton (campbellbarton) added inline comments.Aug 17 2020, 12:19 PM

source/blender/editors/interface/interface_layout.c
4429–4443	Note that it's possible to forward extra arguments to `LISTBASE_FOREACH` into the end of the for-loop using `__VA_ARGS__`, although this can get a bit involved, since in most cases we only want an extra counter, it's probably not such an advantage.

Hans Goudey (HooglyBoogly) added inline comments.Aug 17 2020, 3:31 PM

source/blender/editors/interface/interface_layout.c
4429–4443	I like the idea of having a `LISTBASE_FOREACH_INDEX` macro, but I think to keep the scope of the index variable confined to the loop we would need a `LISTBASE_FOREACH_INDEX_END` macro to close off the scope. Unless the macro doesn't declare the index variable. Then we could use this: #define LISTBASE_FOREACH_INDEX(type, var, list, index_var) \ for (type var = (type)((list)->first); var != NULL; \ var = (type)(((Link *)(var))->next), index_var++) \

We could initialize & increment, but not define, matching BM_ITER_MESH_INDEX:

diff --git a/.clang-format b/.clang-format
index cb5cdab496f..ef603a92288 100644
--- a/.clang-format
+++ b/.clang-format
@@ -235,6 +235,7 @@ ForEachMacros:
   - LISTBASE_CIRCULAR_BACKWARD_BEGIN
   - LISTBASE_CIRCULAR_FORWARD_BEGIN
   - LISTBASE_FOREACH
+  - LISTBASE_FOREACH_INDEX
   - LISTBASE_FOREACH_BACKWARD
   - LISTBASE_FOREACH_MUTABLE
   - LISTBASE_FOREACH_BACKWARD_MUTABLE
diff --git a/source/blender/blenkernel/intern/anim_sys.c b/source/blender/blenkernel/intern/anim_sys.c
index 5b5e32f1d81..d8ccca464e9 100644
--- a/source/blender/blenkernel/intern/anim_sys.c
+++ b/source/blender/blenkernel/intern/anim_sys.c
@@ -2783,9 +2783,9 @@ void BKE_animsys_update_driver_array(ID *id)
     int num_drivers = BLI_listbase_count(&adt->drivers);
     adt->driver_array = MEM_mallocN(sizeof(FCurve *) * num_drivers, "adt->driver_array");
 
-    int driver_index = 0;
-    LISTBASE_FOREACH (FCurve *, fcu, &adt->drivers) {
-      adt->driver_array[driver_index++] = fcu;
+    int driver_index;
+    LISTBASE_FOREACH_INDEX (FCurve *, fcu, &adt->drivers, driver_index) {
+      adt->driver_array[driver_index] = fcu;
     }
   }
 }
diff --git a/source/blender/blenlib/BLI_listbase.h b/source/blender/blenlib/BLI_listbase.h
index fa7cf7a1847..634546e614e 100644
--- a/source/blender/blenlib/BLI_listbase.h
+++ b/source/blender/blenlib/BLI_listbase.h
@@ -171,6 +171,10 @@ struct LinkData *BLI_genericNodeN(void *data);
 #define LISTBASE_FOREACH(type, var, list) \
   for (type var = (type)((list)->first); var != NULL; var = (type)(((Link *)(var))->next))
 
+#define LISTBASE_FOREACH_INDEX(type, var, list, index_var) \
+  for (type var = (((void)(index_var = 0)), (type)((list)->first)); var != NULL; \
+       var = (type)(((Link *)(var))->next), index_var++)
+
 #define LISTBASE_FOREACH_BACKWARD(type, var, list) \
   for (type var = (type)((list)->last); var != NULL; var = (type)(((Link *)(var))->prev))