Re: [PATCH] Improve Configuration Checking and Parsing

Thanks Alex. I'm out of town during the weekend.

Here's the refined patch. I addressed all the issues you pointed out.

BTW, how to identify these TAB and WHITESPACE issues by search the
patch? (we cannot search WHITESPACE, right? :P)

Thanks a lot!
Tianyin

On Fri, Jan 11, 2013 at 4:18 PM, Alex Rousskov
<rousskov_at_measurement-factory.com> wrote:
> 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.
>

-- 
Tianyin XU,
http://cseweb.ucsd.edu/~tixu/