Re: [PATCH] Improve Configuration Checking and Parsing

[Amos Jeffries]

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
<rouss...@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
<rouss...@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
<rouss...@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.