commons-dev mailing list archives

Site index · List index
Message view « Date » · « Thread »
Top « Date » · « Thread »
From sebb <seb...@gmail.com>
Subject Re: svn commit: r1523175 - /commons/proper/jxpath/trunk/src/java/org/apache/commons/jxpath/JXPathContext.java
Date Sat, 14 Sep 2013 12:16:04 GMT
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?

> 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


Mime
View raw message