You are viewing a plain text version of this content. The canonical link for it is here.
Posted to dev@commons.apache.org by sebb <se...@gmail.com> on 2013/09/14 11:44:57 UTC
Re: svn commit: r1523175 - /commons/proper/jxpath/trunk/src/java/org/apache/commons/jxpath/JXPathContext.java
On 14 September 2013 03:13, <gg...@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?
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
Re: svn commit: r1523175 - /commons/proper/jxpath/trunk/src/java/org/apache/commons/jxpath/JXPathContext.java
Posted by Gary Gregory <ga...@gmail.com>.
On Sat, Sep 14, 2013 at 8:16 AM, sebb <se...@gmail.com> wrote:
> On 14 September 2013 13:09, Gary Gregory <ga...@gmail.com> wrote:
> > On Sat, Sep 14, 2013 at 5:44 AM, sebb <se...@gmail.com> wrote:
> >
> >> On 14 September 2013 03:13, <gg...@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
Re: svn commit: r1523175 - /commons/proper/jxpath/trunk/src/java/org/apache/commons/jxpath/JXPathContext.java
Posted by sebb <se...@gmail.com>.
On 14 September 2013 13:09, Gary Gregory <ga...@gmail.com> wrote:
> On Sat, Sep 14, 2013 at 5:44 AM, sebb <se...@gmail.com> wrote:
>
>> On 14 September 2013 03:13, <gg...@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
Re: svn commit: r1523175 - /commons/proper/jxpath/trunk/src/java/org/apache/commons/jxpath/JXPathContext.java
Posted by Gary Gregory <ga...@gmail.com>.
On Sat, Sep 14, 2013 at 5:44 AM, sebb <se...@gmail.com> wrote:
> On 14 September 2013 03:13, <gg...@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.
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