On 11/03/2009, Rainer Jung <rainer.j...@kippdata.de> wrote:
> Hi Sebb,
>
> On 10.03.2009 21:33, sebb wrote:
>
> > On 10/03/2009, Rainer Jung<rainer.j...@kippdata.de> wrote:
> >
> > > Hi all,
> > >
> > > version 1.2.28 of mod_jk is approaching its release. A code snapshot
> > > (revision 752124) is available at:
> > >
> > >
> http://tomcat.apache.org/dev/dist/tomcat-connectors/jk/source/jk-1.2.28-dev/
> > >
> > >
> >
> > The NOTICE file is non-standard, as it does not contain the product
> > name or the Copyright year(s).
> >
> > See:
> >
> > http://www.apache.org/legal/src-headers.html#notice
> >
> > for details
> >
>
> Fixed in r752627.
OK.
>
> > ===
> >
> > Incorrect Javadoc:
> >
> org.apache.jk.status.JkStatusTask.setPropertyBalancer
> >
>
> Fixed in r752629 +r752644. The whole JavaDocs are a mess though, they are
> more or less only stubs. The Java parts of the connectors release would need
> a bit of love.
OK, point taken.
>
> > ===
> >
> > Findbugs reports:
> > org.apache.jk.status.JkStatusParser.digester isn't final
> but should be
> >
>
> I asked Peter to have a look.
>
Good.
Shared mutable static fields are generally a bad idea.
Even private fields are better made final if possible.
>
> > ===
> >
> > Some odd numbers in the code in mod_jk.c:
> >
> > static array_header *parse_request_log_string(pool * p, const char *s,
> > const char **err)
> > {
> > array_header *a = ap_make_array(p, 15,
> sizeof(request_log_format_item));
> > // Why 15?
> > ...
> >
>
> Initial size, the array will automatically grow, if needed. I first
Oh, I see.
> introduced a defined constant for that, and then changed my mind and
> switched to using "0" as the initial size. Arrays and tables resize
> automatically and this is not a critical code path w.r.t. performance.
OK.
<soapbox>
I'm always wary about numbers in code - anything other than +/-1 or 0
has the potential to be wrong or misunderstood, especially if it is
not documented.
There was recent Tomcat bug to do with timeouts where there was a bare
number 100000 or was it 1000000? The meaning of number was/is not
documented, so it's not trivial to check if it's correct.
Much easier to understand would be something like
#define 100MILLISECS_AS_MICROSECS 100000
I'd like to see all numbers (apart from 0/1) expressed as defines, but
that is perhaps asking too much.
Helps document the code, and makes it much easier to change later, as
one can distinguish numbers with the same value but different
functions.
</soapbox>
> r752636 + r752641.
>
> > static const char *jk_add_env_var(cmd_parms * cmd,
> > void *dummy,
> > char *env_name, char *default_value)
> > {
> > ...
> > conf->envvar_items = ap_make_array(cmd->pool, 0,
> > sizeof(envvar_item));
> > // Why 0? This is the number of entries to create?
> >
>
> Default + automatic resize. See above.
>
> Thanks for having a look.
>
> Regards,
>
> Rainer
>
>
> ---------------------------------------------------------------------
> To unsubscribe, e-mail: dev-unsubscr...@tomcat.apache.org
> For additional commands, e-mail: dev-h...@tomcat.apache.org
>
>
---------------------------------------------------------------------
To unsubscribe, e-mail: dev-unsubscr...@tomcat.apache.org
For additional commands, e-mail: dev-h...@tomcat.apache.org
