Re: [PATCH] Improve Configuration Checking and Parsing

On 01/11/2013 01:06 AM, Tianyin Xu wrote:
> Hi, Amos, Alex,
>
> Thanks a lot for your comments! They are really good advice.
>
> The updated patch is attached. It addresses all the issues you
> mentioned except whether to deprecate the "enable"/"disable" options,
> which is worth of discussion.
>
> Thanks,
> Tianyin
>
>
> On Thu, Jan 10, 2013 at 8:14 AM, Alex Rousskov
> <rousskov_at_measurement-factory.com> wrote:
>> On 01/05/2013 05:12 PM, Tianyin Xu wrote:
>>
>>> @@ -193,6 +290,8 @@
>>> return false;
>>> } else if ((port = strtol(token, &tmp, 10)), !*tmp) {
>>> /* port */
>>> + port = xatos(token);
>>> +
>>> } else {
>>> host = token;
>>> port = 0;
>>
>> Please do not set "port" twice because it is confusing and the comma
>> operator in the expression only makes things worse. Do something like
>> this instead:
>>
>> else if (strtol(token, &tmp, 10) && !*tmp) {
>> port = xatos(token);
>> }

> + } else if (strtol(token, &tmp, 10), !*tmp) {
> /* port */
> + port = xatos(token);
> +
> } else {

Please replace the comma operator with the "&&" operator.

Please remove the empty line added in the patch.

Please remove the /* port */ comment as no longer needed.

> - } else if ((port = strtol(token, &junk, 10)), !*junk) {
> + } else if (strtol(token, &junk, 10), !*junk) {
> /* port */
> + port = xatos(token);

Same comments apply here, except there is no extra empty line to remove.

> while (port && i < WCCP2_NUMPORTS) {
> - p = strtol(port, &end, 0);
> + p = xatoi(port);TABHERE
>

> +unsigned int
> +xatoui(const char *token)
> +{
> + int64_t input = xatoll(token, 10);
> + if (input < 0) {
> +TABHEREdebugs(0, DBG_PARSE_NOTE(DBG_IMPORTANT), "ERROR: The input value '" << token << "' cannot be less than 0.");
> + self_destruct();
> + }

Please replace the leading tab with spaces and remove trailing
whitespace. BTW, such things are easy to find by searching the patch.

The above polishing touches can be done during commit IMO if you do not
want to resubmit.

Thanks a lot,

Alex.
Received on Sat Jan 12 2013 - 00:18:50 MST