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

[Casper Hornstrup]

>   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 
Yes, that is definitely one for the wall of shame ;-)

Casper

[Gunnar Dalsnes]

> >  Such a source
> > is simply unreadable.
> >
> > - Hartmut 
>  
>  
>  Yes, that is definitely one for the wall of shame ;-)
>   
You are all crazy.

[Gunnar Dalsnes]

>   
>  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. 
Yes? And how is this different from a normal for loop?????????