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. > + { > +_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