commons-dev mailing list archives

Site index · List index
Message view « Date » · « Thread »
Top « Date » · « Thread »
From Matt Benson <gudnabr...@yahoo.com>
Subject Re: [VOTE] Release JXPath 1.3 based on RC3
Date Mon, 16 Jun 2008 15:49:07 GMT

--- sebb <sebbaz@gmail.com> wrote:

> On 16/06/2008, Matt Benson <gudnabrsam@yahoo.com>
> wrote:
> >
> >  --- sebb <sebbaz@gmail.com> wrote:
> >
> >
> > > On 15/06/2008, Oliver Heger
> >  > <oliver.heger@oliver-heger.de> wrote:
> >  > > sebb schrieb:
> >  > >
> >  > > > On 14/06/2008, Matt Benson
> >  > <gudnabrsam@yahoo.com> wrote:
> >  > > >
> >  > > > >  --- Oliver Heger
> >  > <oliver.heger@oliver-heger.de> wrote:
> >  > > > >
> >  > > > >  > +1
> >  > > > >  >
> >  > > > >  > Artifacts look very good. I also ran
> the
> >  > tests for
> >  > > > >  > commons configuration
> >  > > > >  > with the new version successfully.
> >  > > > >  >
> >  > > > >  > The only thing that makes me a bit
> uneasy
> >  > is the
> >  > > > >  > findbugs report showing
> >  > > > >  > 133 errors. Did you have a look at
> those?
> >  > > > >  >
> >  > > > >
> >  > > > >
> >  > > > > I actually didn't, but I don't see
> anything in
> >  > there
> >  > > > >  that really surprises me.  Some false
> >  > positives (e.g.
> >  > > > >  String ==), some Serialization issues I
> knew
> >  > were
> >  > > > >  there.  It would be nice to attack these
> for
> >  > another
> >  > > > >  release.
> >  > > > >
> >  > > >
> >  > > > Certainly some of them need fixing, e.g.
> >  > > >
> >  > > > Use of non-localized String.toUpperCase()
> or
> >  > String.toLowerCase
> >  > > > at
> >  > > >
> >  > >
> >  >
> > 
>
http://people.apache.org/~mbenson/jxpath-1.3-rc3/site/xref/org/apache/commons/jxpath/ri/model/NodePointer.html#549
> >  > > > and
> >  > > >
> >  > >
> >  >
> > 
>
http://people.apache.org/~mbenson/jxpath-1.3-rc3/site/xref/org/apache/commons/jxpath/ri/model/dom/DOMNodePointer.html#330
> >  > > >
> >  > > > These should use something like
> >  > toUpperCase(Locale.ENGLISH).
> >  > > >
> >  > > > Might also be worth adding exclusions for
> the
> >  > bugs that are false
> >  > > positives...
> >  > > >
> >  > > >
> >  > >  I think if the problems were introduced
> during
> >  > the work on the 1.3 release,
> >  > > they really should be addressed. However if
> they
> >  > live in the code base for a
> >  > > longer time, they have obviously not caused
> major
> >  > problems yet, and the
> >  > > strategy to fix them in the next release
> seems
> >  > reasonable to me.
> >  > >
> >  >
> >  > Good point.
> >  >
> >  > Is this one new?
> >  >
> >  > Dead store to collection in
> >  >
> > 
>
org.apache.commons.jxpath.ri.model.beans.CollectionPointer.createPath(JXPathContext)
> >  >
> >  >
> > 
>
http://people.apache.org/~mbenson/jxpath-1.3-rc3/site/xref/org/apache/commons/jxpath/ri/model/beans/CollectionPointer.html#125
> >  >
> >  > The code certainly looks odd ...
> >  >
> >
> >
> > Not new, and from the POV of understanding what
> the
> >  code there does (grows the underlying collection
> to a
> >  size such that the index is valid) doesn't appear
> >  problematic.
> 
> What's confusing is that the collection variable is
> assigned, but not used.
> 

Actually the problem here is shadowing.  I'll work on
this.  This is a bug, but one that would only make a
difference for arrays as they must be reassigned while
a collection would simply be grown.  Not sure if this
should be a blocker.  :|

-Matt

> Perhaps remove the assignment to make it clearer.
> 
> >
> >  > (most of the other dead store reports seem to
> be
> >  > FPs)
> >  >
> >  >
> >  > If getMessage() is heavily used, then this one
> >  > should be fixed:
> >  >
> >  > Method
> >  >
> > 
>
org.apache.commons.jxpath.ri.parser.ParseException.getMessage()
> >  > concatenates strings using + in a loop
> >
> >
> > This is generated code.  It wouldn't hurt to fix
> it,
> >  since the JXPath parser code is generated and
> then
> >  saved, but in theory if we ever regenerated the
> parser
> >  from the JavaCC grammar we'd be starting from
> scratch
> >  to add back any improvements made.  It being the
> case
> >  that the rightness of making changes to generated
> code
> >  is in doubt, I certainly wouldn't think the
> release
> >  should be held up because of it.
> 
> Sorry, did not notice that was generated code.
> There were plenty of other complaints about the
> generated code which I
> did ignore.
> 
> >  So far the only issue I've seen that I would
> think is
> >  terror-worthy is Phil's discovery of my having
> left in
> >  some [io] references when I (relatively) recently
> >  cloned its build instructions for a much-needed
> >  update.  He, however, has given his +1.
> >
> >  In any event, all these issues should be
> addressed at
> >  some point, so I'll go ahead and work on these in
> >  trunk.
> >
> 
> That's fine by me.
> 
> >
> >  -Matt
> >
> >
> >  >
> >  > >  Oliver
> >  > >
> >  > >
> >  > >
> >  > >
> >  > > >
> >  > > > >  Does your +1 still stand?
> >  > > > >
> >  > > > >
> >  > > > >  -Matt
> >  > > > >
> >  > > > >
> >  > > > >  > Oliver
> >  > > > >  >
> >  > > > >  > Matt Benson schrieb:
> >  > > > >  > > Thanks to anyone who reported issues
> with
> >  > the
> >  > > > >  > previous
> >  > > > >  > > two release candidates, and
> especially to
> >  > those
> >  > > > >  > who
> >  > > > >  > > helped resolve them.
> >  > > > >  > >
> >  > > > >  > > The artifacts are here:
> >  > > > >  > >
> >  >
> http://people.apache.org/~mbenson/jxpath-1.3-rc3/
> >  > > > >  > >
> >  > > > >  > > The tag is here:
> >  > > > >  > >
> >  > > > >  >
> >  > > > >
> >  > >
> >  >
> > 
>
http://svn.apache.org/viewvc/commons/proper/jxpath/tags/JXPATH_1_3_RC3/
> >  > > > >  > >
> >  > > > >  > > Site:
> >  > > > >  > >
> >  > > > >  >
> >  > > > >
> >  >
> > 
>
http://people.apache.org/~mbenson/jxpath-1.3-rc3/site
> >  > > > >  > >
> >  > > > >  > > Clirr Report (compared to 1.2;
> one-shot
> >  > not
> >  > > > >  > working w/
> >  > > > >  > > M2)
> >  > > > >  > >
> >  > > > >  >
> >  > > > >
> >  > >
> >  >
> > 
>
http://people.apache.org/~mbenson/jxpath-1.3-rc3/clirr-report.txt
> >  > > > >  > >
> >  > > > >  > > I'd be grateful if you can make time
> to
> >  > check the
> >  > > > >  > > artifacts and cast your vote, which
> will
> >  > be open
> >  > > > >  > at
> >  > > > >  > > least until Friday, June 20.
> >  > > > >  > >
> >  > > > >  > > Thanks,
> >  > > > >  > > Matt
> >  > > > >  > >
> >  > > > >  > >
> >  > > > >  > >
> >  > > > >  > >
> >  > > > >  > >
> >  > > > >  >
> >  > > > >
> >  > >
> >  >
> > 
>
---------------------------------------------------------------------
> >  > > > >  > > To unsubscribe, e-mail:
> >  > > > >  > dev-unsubscribe@commons.apache.org
> >  > > > >  > > For additional commands, e-mail:
> >  > > > >  > dev-help@commons.apache.org
> >  > > > >  > >
> >  > > > >  >
> >  > > > >  >
> >  > > > >  >
> >  > > > >
> >  > >
> >  >
> > 
>
---------------------------------------------------------------------
> >  > > > >  > To unsubscribe, e-mail:
> >  > > > >  > dev-unsubscribe@commons.apache.org
> >  > > > >  > For additional commands, e-mail:
> >  > > > >  > dev-help@commons.apache.org
> >  > > > >  >
> >  > > > >  >
> >  > > > >
> >  > > > >
> >  > > > >
> >  > > > >
> >  > > > >
> >  > > > >
> >  > >
> >  >
> > 
>
---------------------------------------------------------------------
> >  > > > >  To unsubscribe, e-mail:
> >  > > dev-unsubscribe@commons.apache.org
> >  > > > >  For additional commands, e-mail:
> >  > dev-help@commons.apache.org
> >  > > > >
> >  > > > >
> >  > > > >
> >  > > >
> >  > > >
> >  > >
> >  >
> > 
>
---------------------------------------------------------------------
> >  > > > To unsubscribe, e-mail:
> >  > > dev-unsubscribe@commons.apache.org
> >  > > > For additional commands, e-mail:
> >  > dev-help@commons.apache.org
> >  > > >
> >  > > >
> >  > >
> >  > >
> >  > >
> >  >
> > 
>
---------------------------------------------------------------------
> >  > >  To unsubscribe, e-mail:
> >  > dev-unsubscribe@commons.apache.org
> >  > >  For additional commands, e-mail:
> >  > dev-help@commons.apache.org
> >  > >
> >  > >
> >  >
> >  >
> > 
>
---------------------------------------------------------------------
> >  > To unsubscribe, e-mail:
> >  > dev-unsubscribe@commons.apache.org
> >  > For additional commands, e-mail:
> >  > dev-help@commons.apache.org
> >  >
> >  >
> >
> >
> >
> >
> >
> > 
>
---------------------------------------------------------------------
> >  To unsubscribe, e-mail:
> dev-unsubscribe@commons.apache.org
> >  For additional commands, e-mail:
> dev-help@commons.apache.org
> >
> >
> 
>
---------------------------------------------------------------------
> To unsubscribe, e-mail:
> dev-unsubscribe@commons.apache.org
> For additional commands, e-mail:
> dev-help@commons.apache.org
> 
> 



      

---------------------------------------------------------------------
To unsubscribe, e-mail: dev-unsubscribe@commons.apache.org
For additional commands, e-mail: dev-help@commons.apache.org


Mime
View raw message