Thanks, Amos. What do you mean "Just check that unrelated changes it finds caused by other peoples bad patches are left out of your patch."?
What should I do at my side? T On Tue, Jan 15, 2013 at 7:37 PM, Amos Jeffries <[email protected]> wrote: > On 16/01/2013 2:01 p.m., Tianyin Xu wrote: >> >> Sounds great! Thanks a lot, Alex! I'll take care about the formatting >> issues next time. > > > Best way is to get the particular version of astyle which the squid-cache > server uses and run scripts/source-maintenance.sh on your checkout prior to > submitting. That will catch most of the style issues and generate any > automated changes such as profiler and debugs info files which may be > changed by your patch. Just check that unrelated changes it finds caused by > other peoples bad patches are left out of your patch. > > Amos > > >> >> 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/
