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
> <[email protected]> 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
>>> <[email protected]> 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.
>>
>
>
>
