Felix Zielcke wrote:
> Am Dienstag, den 02.09.2008, 00:46 +0200 schrieb Felix Zielcke:
>
>> I'm too lazy now to make a new patch and go sleeping now.
>> [...]
>>
>> I hope that Marco could have a quick look over it especially the
>> changelog part :)
>
> Final patch attached.
> In changelog I had again past and present mixed and I just use now the
> `normal' types instead of grub_uintN_t, seems like that's more used on
> util/
> So if there are no objections I'd like to commit this :)
> + unsigned short i, j;
> + const unsigned char k = sizeof ("/dev/mapper/") - 1;
> + const unsigned short l = strlen (os_dev) - k + 1;
sizeof returns type of size_t so it would be good that char k uses that.
I am a bit surprised that this didn't generate compiler warning? And
there is no reason to define integers as constants unless you really
want to make sure they don't change :)
I assume we have grub_size_t or comparable there.
> + const unsigned short l = strlen (os_dev) - k + 1;
>
> - break;
> + grub_dev = xmalloc (strlen (os_dev) - k + 1);
if you already calculate something to variable it would be nice to
re-use that calculation again.
> + for (i = 0, j = 0; i < l; i++, j++)
> + {
> + grub_dev[j] = os_dev[k + i];
> + if (os_dev[k + i] == '-' && os_dev[k + i + 1] == '-')
> + i++;
> + }
Can't you index i: k <= i < strlen(os_dev)? Would make this a bit more
readable. Or as you could just increment k in every loop and use that to
index. Your 0 <= j < l should limit char array.
You could use a bit more descriptive names like l->len, k->start/offset.
For indexes we quite often use i,j,k.
> + p = strchr (os_dev, 'p');
> + if (p)
> + *p = ',';
It is usually a bad idea to modify source string.
_______________________________________________
Grub-devel mailing list
[email protected]
http://lists.gnu.org/mailman/listinfo/grub-devel
