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

[Hartmut Birr]

gdalsnes(a)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