Re: Updated PedUnit patch

[Andrew Clausen]

Hi Leslie,

Have you run any basic tests on your code?  If it basically works, feel
free to commit what you have to cvs.  We can always change things
afterwards...

On Tue, Jun 28, 2005 at 04:04:16PM +0200, address@hidden wrote:
> > You also need to free *sector and *range, and set them to NULL...
> Fixed. I hope I never forget to free things again when throwing exceptions.

Oops, you shouldn't free *sector.  Only *range.

> > Come to think of it, why is there any rounding here at all?
> > 
> > (That is, why is there a 0.5 in there?)
> I didn't put it in, so I don't know for sure -- but I also pondered about it
> and came to the conclusion that it is a ward against division by zero.

You should always be suspicious of a piece of code you don't understand!
IIRC, this was to make formatting and inputting consistent.  I now think
this code is bogus, so we should just kill it.

> > Do we need the braces?
> I'm afraid yes.

Why?

> > Also, this still spills of 80 characters.  Perhaps define cyl_size at
> > the top?
> I suggest 
> 
> PedSector cyl_size = dev->bios_geom.heads
>                      * dev->bios_geom.sectors;
> 
> instead. This way we save the memory management when we do not need
> this variable.

Who cares about 8 bytes of memory?  Premature optimization is the root
of all evil!  Cleanliness and readability are more important, unless you
are talking about megabytes or seconds.  (This is doubly true for
Parted, because users expect high reliability and don't care much about
speed.)

Besides, it makes no difference.  The C compiler won't allocate any
extra stack space for cyl_size: it will just compute it as needed,
and/or stick it in a register.

> +     {
> +         long long unit_size = ped_unit_get_size (dev, unit);
> +         PedSector unit_sectors = ped_div_round_up (unit_size, 
> PED_SECTOR_SIZE);
> +
> +         *sector = ped_div_round_up (num * unit_size, PED_SECTOR_SIZE);
> +         *range = geometry_from_centre_radius (dev, *sector, unit_sectors);
> +
> +         if (!ped_geometry_test_sector_inside (*range, *sector))
> +         {
> +             ped_free (sector);
> +             ped_free (*range);
> +
> +             ped_exception_throw (
> +                     PED_EXCEPTION_ERROR,
> +                     PED_EXCEPTION_CANCEL,
> +                     _("Sector outside of device"));
> +         }

This is still broken.  I would write it something like this:

> +         if (!ped_geometry_test_sector_inside (*range, *sector)) {
> +             ped_exception_throw (
> +                     PED_EXCEPTION_ERROR, PED_EXCEPTION_CANCEL,
> +                     _("The location %s is outside of the device %s."),
> +                     str, dev->path);
> +             *sector = 0;
> +             ped_free (*range);
> +             *range = NULL;
> +             return 0;
> +         }

> +/**
> + * Returns the byte size of a given unit.
> + */
> +long long
> +ped_unit_get_size (PedDevice* dev, PedUnit unit)
> +{
> +    switch (unit)
> +    {

I notice you have started putting opening braces on their own line.
This is different from the rest of Parted.  (We only put the first
brace of a function on a new line... see the Linux coding standard
for rationale.)  It doesn't matter that much.

> +     case PED_UNIT_SECTOR:       return PED_SECTOR_SIZE;
> +     case PED_UNIT_BYTE:         return 1;
> +     case PED_UNIT_KILOBYTE:     return PED_KILOBYTE_SIZE;
> +     case PED_UNIT_MEGABYTE:     return PED_MEGABYTE_SIZE;
> +     case PED_UNIT_GIGABYTE:     return PED_GIGABYTE_SIZE;
> +     case PED_UNIT_TERABYTE:     return PED_TERABYTE_SIZE;
> +
> +     case PED_UNIT_CYLINDER:
> +         {
> +             PedSector cyl_size = dev->bios_geom.heads
> +                                  * dev->bios_geom.sectors;
> +             return ( cyl_size * PED_SECTOR_SIZE );
> +         }

I think it's better to put the cyl_size definition at the top of the
function.  Why the brackets around the return value?

> +     case PED_UNIT_PERCENT:
> +         return ( get_sectors_per_cent (dev) * PED_SECTOR_SIZE );

Why the brackets?

> @@ -918,8 +905,8 @@ do_print (PedDevice** dev)
>               return status;
>       }
>  
> -     start = ped_unit_format (0, *dev);
> -     end = ped_unit_format ((*dev)->length - 1, *dev);
> +     start = ped_unit_format (*dev,0 );

Bad formatting here ^

> @@ -1197,7 +1184,7 @@ do_resize (PedDevice** dev)
>       PedFileSystem*          fs;
>       PedConstraint*          constraint;
>       PedSector               start, end;
> -     int                     is_start_exact, is_end_exact;
> +     PedGeometry     *range_start, *range_end;
>       PedGeometry             new_geom;

Inconsistent formatting.

Cheers,
Andrew