On 06/07/2009 02:41 PM, Andrea Taverna wrote: > Hi, > > I made a tiny change to string-related code in src/utli/string.c and > /src/util/conv.c, following Pasky's TODO comments regarding > optimization. Below there's the related diff output. > > I feel a bit embarrassed for asking. Since this is the first code I try > to submit for an open source project, I don't have much of a clue on how > to do it properly. What should I do next?
Thank you for your contribution. Here are two general suggestions: 1. Send your patch as an attachment to avoid mangling of white space. 2. Please follow the formatting style used in the project, which is outlined in doc/hacking.txt. In particular, put declarations only at the start of a block, and indent and place braces as is done in the existing code. > > regards, > > Andrea > > ==============DIFF OUTPUT ======================== > diff -ru old/elinks/src/util/conv.c elinks/src/util/conv.c > --- old/elinks/src/util/conv.c 2009-06-07 14:44:19.000000000 +0200 > +++ elinks/src/util/conv.c 2009-06-07 20:37:47.000000000 +0200 > @@ -340,17 +340,34 @@ > return string; > } > > -/* TODO Optimize later --pasky */ > + > struct string * > add_quoted_to_string(struct string *string, const unsigned char *src, > int len) > { > - for (; len; len--, src++) { > - if (isquote(*src) || *src == '\\') > - add_char_to_string(string, '\\'); > - add_char_to_string(string, *src); add_char_to_string asserts that neither pointer is NULL and that the second string is not empty. It also calls check_string_magic. add_quoted_to_string should probably perform these checks once. > - } > > - return string; > + if (len > 0) > + { Place the open brace on the same line as the if clause, and indent using tabs. > + const unsigned char *c; > + int i; > + unsigned int old_length = string->length; > + > + Avoid trailing white space (you have a tab on the second blank line there). > + for(i = len, c = src; i; i--, c++){ > + if (isquote(*c) || *c =='\\') > + string->length++; > + string->length++; > + }; > + > + realloc_string(string, string->length); > + enforce_null_terminated(*string); You forgot to include the definition of enforce_null_terminated in your patch. (I do stuff like that all the time.) > + > + for (i = old_length, c = src ; i < string->length; i++, c++) { > + if (isquote(*src) || *c == '\\') > + string->source[i++] = '\\'; > + string->source[i]= (*c); Please put spaces around the '=', and delete the extra parentheses around '*c'. > + } > + }; > + return string; > } > > struct string * It looks like the only code that uses add_quoted_to_string is the options saving code and the options manager (via the write() callbacks in the options_types structure), and saving with config.saving_style = 1 is already instantaneous for me, so it isn't all that time critical. Aside from that, have you profiled to see whether there is a measurable increase in performance? Micro-optimisations don't always have the expected results. I would guess that trading a bunch of superfluous NUL-byte assignments, calls to check_string_magic, and re-allocations for an extra loop will improve performance, but I'm curious as to whether it is a substantial improvement. It might be a better trade-off to leave this relatively seldom used routine in the old, smaller and slightly more comprehensible form. > diff -ru old/elinks/src/util/string.c elinks/src/util/string.c > --- old/elinks/src/util/string.c 2009-06-07 14:44:19.000000000 +0200 > +++ elinks/src/util/string.c 2009-06-07 20:39:20.000000000 +0200 > @@ -294,9 +294,6 @@ > > /* The new string utilities: */ > > -/* TODO Currently most of the functions use add_bytes_to_string() as a > backend > - * instead we should optimize each function. */ > - > NONSTATIC_INLINE struct string * > #ifdef DEBUG_MEMLEAK > init_string__(const unsigned char *file, int line, struct string *string) > @@ -351,7 +348,16 @@ > > if (!*source) return string; > > - return add_bytes_to_string(string, source, strlen(source)); > + size_t newlength = string->length + strlen(source); > + > + realloc_string(string, newlength); > + > + memcpy(&(string->source[string->length]), source, newlength - > string->length); > + > + string->length = newlength; > + string->source[string->length] = '\0'; > + > + return string; > } > > /** @relates string */ > @@ -385,7 +391,16 @@ > > if (!from->length) return string; /* optimization only */ > > - return add_bytes_to_string(string, from->source, from->length); > + size_t newlength = string->length + from->length; > + > + realloc_string(string, newlength); > + > + memcpy(&(string->source[string->length]), from->source, newlength - > string->length); > + > + string->length = newlength; > + string->source[string->length] = '\0'; > + > + return string; > } > > /** @relates string */ If the compiler inlines add_bytes_to_string, I don't see any difference between the old add_to_string and add_string_to_string and the new (besides debugging code, which can be eliminated by configuring with CONFIG_DEBUG undefined and CONFIG_FASTMEM defined). Can you explain or use a profiler to show any difference in running time? I don't know what pasky had in mind with his comment about optimising this code. Thanks for taking the time to write these patches. I hope that my comments are helpful. Best regards, -- Miciah Masters <miciah.mast...@gmail.com> / <mdm0...@ecu.edu> _______________________________________________ elinks-dev mailing list elinks-dev@linuxfromscratch.org http://linuxfromscratch.org/mailman/listinfo/elinks-dev