Em Mon, Oct 08, 2012 at 09:43:33AM +0300, Irina Tirdea escreveu: > > +/* Group the digits according to the grouping rules of the current locale. > + The interpretation of GROUPING is as in `struct lconv' from <locale.h>. > */ > +static int group_number_locale(char *number, char **gnumber) > +{ > + const char *thousands_sep = NULL, *grouping = NULL; > + int glen, tlen, dest_alloc_size, src_size, ret = 0, cnt;
Please set ret to -ENOMEM; and... > + char *dest_alloc_ptr, *dest_end, *src_start, *src_end; > + When we have else clauses, I think its better to use #ifdef LOCALE_SUPPORT struct lconv *lc = localeconv(); if (lc != NULL) { thousands_sep = lc->thousands_sep; grouping = lc->grouping; } #else thousands_sep = ","; grouping = "\x3"; #endif > +#ifndef LOCALE_SUPPORT > + thousands_sep = ","; > + grouping = "\x3"; > +#else > + struct lconv *lc = localeconv(); > + if (lc != NULL) { > + thousands_sep = lc->thousands_sep; > + grouping = lc->grouping; > + } > +#endif > + > + *gnumber = NULL; > + /* No grouping */ > + if (thousands_sep == NULL || grouping == NULL || > + *thousands_sep == '\0' || *grouping == CHAR_MAX || *grouping <= 0) { > + *gnumber = strdup(number); > + if (*gnumber == NULL) > + ret = -ENOMEM; > + goto out; Humm, so we bail out unconditionally? :-) this should be: if (*gnumber == NULL) goto out; > + } > + > + glen = *grouping++; > + tlen = strlen(thousands_sep); > + > + src_size = strlen(number); > + /* Worst case scenario we have 1-character grouping */ > + dest_alloc_size = (src_size + src_size * tlen) * sizeof(char); > + dest_alloc_ptr = zalloc(dest_alloc_size); > + if (dest_alloc_ptr == NULL) { > + ret = -ENOMEM; remove the above > + goto out; > + } > + /* -1 for '\0' */ > + dest_end = dest_alloc_ptr + dest_alloc_size - 1; > + > + src_start = number; > + src_end = number + src_size; > + > + while (src_end > src_start) { > + *--dest_end = *--src_end; > + if (--glen == 0 && src_end > src_start) { > + /* A new group */ > + cnt = tlen; > + do > + *--dest_end = thousands_sep[--cnt]; > + while (cnt > 0); > + > + if (*grouping == CHAR_MAX || *grouping < 0) { > + /* No further grouping to be done. > + Copy the rest of the number. */ > + do > + *--dest_end = *--src_end; > + while (src_end > src_start); > + break; > + } else if (*grouping != '\0') { > + glen = *grouping++; > + } else { > + /* The previous grouping repeats ad infinitum */ > + glen = grouping[-1]; > + } > + } > + } > + > + /* Make a copy with the exact needed size of the grouped number */ > + *gnumber = strdup(dest_end); > + if (*gnumber == NULL) { > + ret = -ENOMEM; ditto > + goto out_free_dest; > + } > + > + /* fall through */ Why the "fall through" comment? This is a common construct and this comment mostly applies where one would expect a break in a switch statement :-) Ah, here you do: ret = 0; Since all went well. > +out_free_dest: > + free(dest_alloc_ptr); > +out: > + return ret; > +} -- To unsubscribe from this list: send the line "unsubscribe linux-kernel" in the body of a message to majord...@vger.kernel.org More majordomo info at http://vger.kernel.org/majordomo-info.html Please read the FAQ at http://www.tux.org/lkml/