Re: [PATCH] Improve Configuration Checking and Parsing

Sounds great! Thanks a lot, Alex! I'll take care about the formatting
issues next time.

Best,
T

On Mon, Jan 14, 2013 at 11:17 AM, Alex Rousskov
<rousskov_at_measurement-factory.com> wrote:
> On 01/14/2013 11:55 AM, Tianyin Xu wrote:
>
>> BTW, how to identify these TAB and WHITESPACE issues by search the
>> patch?
>
> There are many ways. I usually search for the tab character and trailing
> space (" $") when looking at the patch file with "less" before sending
> the patch file to the mailing list, but I suspect there are better ways
> to do this.
>
> Configuring your editor to insert spaces instead of a tab character may
> also be a good idea.
>
>
> Cheers,
>
> Alex.
>
>
>
>> 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/