Re: cdrom_check_status routine in ide-cd.c #if ! STANDARD_ATAPI

[Erik Mouw <J.A.K.Mouw@its.tudelft.nl>]

On Sun, Feb 02, 2003 at 05:00:08PM -0800, Marty Biznatch wrote:
> In the cdrom_check_status routine there is the code:
> 
> static int cdrom_check_status(ide_drive_t *drive,
> struct request_sense *sense)
> 2063 {
> 2064         struct request req;
> 2065         struct cdrom_info *info =
> drive->driver_data;
> 2066         struct cdrom_device_info *cdi =
> &info->devinfo;
> 2067 
> 2068         cdrom_prepare_request(&req);
> 2069 
> 2070         req.sense = sense;
> 2071         req.cmd[0] = GPCMD_TEST_UNIT_READY;
> 2072 
> 2073 #if ! STANDARD_ATAPI
> 2074         /* the Sanyo 3 CD changer uses byte 7 of
> TEST_UNIT_READY to 
> 2075            switch CDs instead of supporting the
> LOAD_UNLOAD opcode   */
> 2076 
> 2077         req.cmd[7] = cdi->sanyo_slot % 3;
> 2078 #endif /* not STANDARD_ATAPI */
> 2079 
> 2080         return cdrom_queue_packet_command(drive,
> &req);
> 2081 }
> 
> Would this code be more readable and perhaps more
> corectly written if the structures cdrom_info and
> cdrom_device_info were conditionaly assembled as the
> #if ! STANDARD_ATAPI line is?
> The optimizer will not allocate space for these
> structures if unused, however it is more code that
> needs to be greped through to understand a routine
> that otherwise could be ignored? The negative of
> course being that conditional ifs are ugly.

Memory space is not an issue, code readability is. You'd rather do
something like this in the header file:

#ifndef STANDARD_ATAPI
#define sanyo_fixup() \
do { \
	req.cmd[7] = cdi->sanyo_slot % 3; \
} while(0)
#else
#define sanyo_fixup() do { } while(0);
#endif

So the code can become:

	sanyo_fixup();

Which gcc will nicely optimise away and is much more readable.


Erik

-- 
J.A.K. (Erik) Mouw
Email: J.A.K.Mouw@its.tudelft.nl  mouw@nl.linux.org
WWW: http://www-ict.its.tudelft.nl/~erik/

PGP signature