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 <[email protected]> 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 >> <[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. >>> >> >> >> > -- Tianyin XU, http://cseweb.ucsd.edu/~tixu/
