[Top][All Lists]
[Date Prev][Date Next][Thread Prev][Thread Next][Date Index][Thread Index]
Re: Updated PedUnit patch
From: |
Andrew Clausen |
Subject: |
Re: Updated PedUnit patch |
Date: |
Tue, 28 Jun 2005 20:03:22 +1000 |
User-agent: |
Mutt/1.5.6+20040907i |
On Tue, Jun 28, 2005 at 01:52:35AM +0200, Leslie Patrick Polzer wrote:
> > Well, you have to type "libtoolize". I guess we could put a shell
> > script in CVS that runs aclocal, libtoolize, autoconf and automake
> > as necessary.
> If you want to go on that track, we might as well remove
> 'configure' and 'Makefile.in' (recursively) from CVS.
Agreed.
> > if (!ped_geometry_test_sector_inside (*range, *sector))
> Why is this necessary?
I see you included correctly it in your patch... it's needed to
test if the user type something a long way outside of the disk,
beyond the error in the unit size.
> > [local variables and line length problems]
> Fixed.
> I tried to put the local variable stuff into a separate function,
I'm not sure what you mean by "local variable stuff".
> but this way we'd end up passing all ped_unit_parse_custom arguments
> to it.
> > This arithmetic looks complicated. I think a helper function to do
> > rounding is in order.
> Done.
See my comments in your code below.
> Proposal: change the signature of
>
> ped_unit_format_custom (PedSector sector, PedDevice* dev, PedUnit unit)
>
> to
>
> ped_unit_format_custom (PedDevice* dev, PedSector sector, PedUnit unit)
Fine by me.
> + {
> + 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_exception_throw (
> + PED_EXCEPTION_ERROR,
> + PED_EXCEPTION_CANCEL,
> + _("Sector outside of device"));
You also need to free *sector and *range, and set them to NULL...
> @@ -404,3 +399,61 @@ ped_unit_parse_custom (const char* str,
> return 1;
> }
>
> +
> +static long long
> +get_sectors_per_cent (PedDevice* dev)
> +{
> + return (long long)( 1 / (100.0 * (dev->length - 1) + 0.5) );
> +}
This isn't what I had in mind. I was hoping for some kind of generic
division-with-rounding function. Something like ped_div_round_up(), but
for floats?
Come to think of it, why is there any rounding here at all?
(That is, why is there a 0.5 in there?)
> +long long
> +ped_unit_get_size (PedDevice* dev, PedUnit unit)
> +{
> + switch (unit)
> + {
> + case PED_UNIT_SECTOR:
> + return PED_SECTOR_SIZE;
> +
> + case PED_UNIT_BYTE:
> + return 1;
> +
> + case PED_UNIT_KILOBYTE:
> + return PED_KILOBYTE_SIZE;
Any objections to this instead:
> + 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_CYLINDER:
> + {
> + PedSector cyl_size = dev->bios_geom.heads *
> dev->bios_geom.sectors;
> + return ( cyl_size * PED_SECTOR_SIZE );
> + }
Do we need the braces?
Also, this still spills of 80 characters. Perhaps define cyl_size at
the top?
Cheers,
Andrew
- Re: Updated PedUnit patch, Leslie Patrick Polzer, 2005/06/27
- Re: Updated PedUnit patch,
Andrew Clausen <=
- Re: Updated PedUnit patch, leslie . polzer, 2005/06/28
- Re: Updated PedUnit patch, Andrew Clausen, 2005/06/28
- Re: Updated PedUnit patch, Andrew Clausen, 2005/06/29
- Re: Updated PedUnit patch, leslie . polzer, 2005/06/29
- Re: Updated PedUnit patch, Andrew Clausen, 2005/06/29
- Re: Updated PedUnit patch, leslie . polzer, 2005/06/29
- Re: Updated PedUnit patch, Andrew Clausen, 2005/06/29
- Message not available
- Re: Updated PedUnit patch, leslie . polzer, 2005/06/29