Re: [PATCH] Improve Configuration Checking and Parsing

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
> <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.
>>>>
>>>
>>>
>
>
Received on Wed Jan 16 2013 - 03:37:49 MST