commons-dev mailing list archives

Site index · List index
Message view « Date » · « Thread »
Top « Date » · « Thread »
From Gary Gregory <garydgreg...@gmail.com>
Subject Re: svn commit: r1523175 - /commons/proper/jxpath/trunk/src/java/org/apache/commons/jxpath/JXPathContext.java
Date Sat, 14 Sep 2013 13:54:39 GMT
On Sat, Sep 14, 2013 at 8:16 AM, sebb <sebbaz@gmail.com> wrote:

> On 14 September 2013 13:09, Gary Gregory <garydgregory@gmail.com> wrote:
> > On Sat, Sep 14, 2013 at 5:44 AM, sebb <sebbaz@gmail.com> wrote:
> >
> >> On 14 September 2013 03:13,  <ggregory@apache.org> wrote:
> >> > Author: ggregory
> >> > Date: Sat Sep 14 02:13:01 2013
> >> > New Revision: 1523175
> >> >
> >> > URL: http://svn.apache.org/r1523175
> >> > Log:
> >> > No need to lazy-init statics contextFactory and compilationContext,
> make
> >> them final.
> >>
> >> Is the class always needed by applications?
> >>
> >
> > Yes, you always need a JXPathContext.contextFactory, from
> > https://commons.apache.org/proper/commons-jxpath/*:*
> >
> > Address address =
> >
> (Address)JXPathContext.newContext(vendor).getValue("locations[address/zipCode='90210']/address");
> >
> > The above could avoid casting with generics (assuming the call site
> > knows what it is doing of course).
> >
> > For JXPathContext.compilationContext, it depends on whether the app
> > calls JXPathContext.compile(String), but I prefer consistency of the
> > init pattern here... It's debatable, sure.
>
> If compilationContext is not always needed, then the patch changes the
> behaviour.
> Using IODH would avoid changing the behaviour, but is slightly more
> involved.
>
> What about the question of the order of the init?
> Why is that important?
>

Because one uses the value of the other.

Gary


>
> > Gary
> >
> >
> >
> >
> >
> >>
> >> If not, then using IODH would be better.
> >>
> >> > Modified:
> >> >
> >>
> commons/proper/jxpath/trunk/src/java/org/apache/commons/jxpath/JXPathContext.java
> >> >
> >> > Modified:
> >>
> commons/proper/jxpath/trunk/src/java/org/apache/commons/jxpath/JXPathContext.java
> >> > URL:
> >>
> http://svn.apache.org/viewvc/commons/proper/jxpath/trunk/src/java/org/apache/commons/jxpath/JXPathContext.java?rev=1523175&r1=1523174&r2=1523175&view=diff
> >> >
> >>
> ==============================================================================
> >> > ---
> >>
> commons/proper/jxpath/trunk/src/java/org/apache/commons/jxpath/JXPathContext.java
> >> (original)
> >> > +++
> >>
> commons/proper/jxpath/trunk/src/java/org/apache/commons/jxpath/JXPathContext.java
> >> Sat Sep 14 02:13:01 2013
> >> > @@ -380,8 +380,17 @@ import org.apache.commons.jxpath.util.Ke
> >> >   * @version $Revision$ $Date$
> >> >   */
> >> >  public abstract class JXPathContext {
> >> > -    private static volatile JXPathContextFactory contextFactory;
> >> > -    private static volatile JXPathContext compilationContext;
> >> > +
> >> > +       static {
> >> > +               // Initialize in this order:
> >>
> >> Why is it necessary to use this order?
> >> The reason should be documented.
> >>
> >> > +               // 1) contextFactory
> >> > +           contextFactory = JXPathContextFactory.newInstance();
> >> > +               // 2) compilationContext
> >> > +           compilationContext = JXPathContext.newContext(null);
> >> > +       }
> >> > +
> >> > +    private static final JXPathContextFactory contextFactory;
> >> > +    private static final JXPathContext compilationContext;
> >>
> >> I'm suprised that the compiler does not complain that the fields are
> >> not defined before they are used in the static{} block.
> >>
> >> I'd prefer to see the definitions before the static block.
> >>
> >> >      private static final PackageFunctions GENERIC_FUNCTIONS =
> >> >          new PackageFunctions("", null);
> >> > @@ -435,9 +444,6 @@ public abstract class JXPathContext {
> >> >       * @return JXPathContextFactory
> >> >       */
> >> >      private static JXPathContextFactory getContextFactory () {
> >> > -        if (contextFactory == null) {
> >> > -            contextFactory = JXPathContextFactory.newInstance();
> >> > -        }
> >> >          return contextFactory;
> >> >      }
> >> >
> >> > @@ -646,9 +652,6 @@ public abstract class JXPathContext {
> >> >       * @return CompiledExpression
> >> >       */
> >> >      public static CompiledExpression compile(String xpath) {
> >> > -        if (compilationContext == null) {
> >> > -            compilationContext = JXPathContext.newContext(null);
> >> > -        }
> >> >          return compilationContext.compilePath(xpath);
> >> >      }
> >> >
> >> >
> >> >
> >>
> >> ---------------------------------------------------------------------
> >> To unsubscribe, e-mail: dev-unsubscribe@commons.apache.org
> >> For additional commands, e-mail: dev-help@commons.apache.org
> >>
> >>
> >
> >
> > --
> > E-Mail: garydgregory@gmail.com | ggregory@apache.org
> > Java Persistence with Hibernate, Second Edition<
> http://www.manning.com/bauer3/>
> > JUnit in Action, Second Edition <http://www.manning.com/tahchiev/>
> > Spring Batch in Action <http://www.manning.com/templier/>
> > Blog: http://garygregory.wordpress.com
> > Home: http://garygregory.com/
> > Tweet! http://twitter.com/GaryGregory
>
> ---------------------------------------------------------------------
> To unsubscribe, e-mail: dev-unsubscribe@commons.apache.org
> For additional commands, e-mail: dev-help@commons.apache.org
>
>


-- 
E-Mail: garydgregory@gmail.com | ggregory@apache.org
Java Persistence with Hibernate, Second Edition<http://www.manning.com/bauer3/>
JUnit in Action, Second Edition <http://www.manning.com/tahchiev/>
Spring Batch in Action <http://www.manning.com/templier/>
Blog: http://garygregory.wordpress.com
Home: http://garygregory.com/
Tweet! http://twitter.com/GaryGregory

Mime
  • Unnamed multipart/alternative (inline, None, 0 bytes)
View raw message