[ros-dev] Re: [ros-diffs] [gdalsnes] 18113: -reorder InsertXscendingOrder macro argument order and update uses

[Hartmut Birr]

gdalsnes at svn.reactos.com wrote:

> *Modified: trunk/reactos/include/reactos/helper.h*
>
>--- trunk/reactos/include/reactos/helper.h	2005-09-26 22:06:31 UTC (rev 18112)
>+++ trunk/reactos/include/reactos/helper.h	2005-09-26 22:57:48 UTC (rev 18113)
>@@ -18,17 +18,21 @@
>
> #define EXPORTED __declspec(dllexport)
> #define IMPORTED __declspec(dllimport)
> 
>  
>
>-/* iterate through the list using a list entry */
>  
>
>+/* iterate through the list using a list entry. 
>+ * elem is set to NULL if the list is run thru without breaking out or if list is empty.
>+ */
>  
>
> #define LIST_FOR_EACH(elem, list, type, field) \
>     for ((elem) = CONTAINING_RECORD((list)->Flink, type, field); \
>  
>
>-         &(elem)->field != (list); \
>  
>
>+         &(elem)->field != (list) || (elem = NULL); \
>  
>
>          (elem) = CONTAINING_RECORD((elem)->field.Flink, type, field))
>          
>  
>
Hi,

IMHO, this is very bad code. You use a macro to hide a 'for' loop, and
later in the source you use a 'break', 'continue' or 'goto' statement to
leave the loop or to bypass some code.

        /* Find entry with SortKey greater than or equal to the
passed-in SortKey */
        LIST_FOR_EACH(ReturnEntry, &DeviceQueue->DeviceListHead,
KDEVICE_QUEUE_ENTRY, DeviceListEntry)
        {
            /* Check if keys match */
            if (ReturnEntry->SortKey >= SortKey) {
               /* We found it, so just remove it */
               RemoveEntryList(&ReturnEntry->DeviceListEntry);
               break;
            }
        }

Such a source is simply unreadable.

- Hartmut