You are viewing a plain text version of this content. The canonical link for it is here.
Posted to issues@commons.apache.org by "Adrian Cumiskey (Created) (JIRA)" <ji...@apache.org> on 2011/10/27 05:49:32 UTC
[jira] [Created] (OGNL-31) [PATCH] Some CPD fixes around ASTMethod
and ASTStaticMethod.
[PATCH] Some CPD fixes around ASTMethod and ASTStaticMethod.
------------------------------------------------------------
Key: OGNL-31
URL: https://issues.apache.org/jira/browse/OGNL-31
Project: OGNL
Issue Type: Improvement
Reporter: Adrian Cumiskey
Priority: Minor
Attachments: patch-OGNL31.txt
I discovered a number of PMD CPD issues around ASTMethod and ASTStaticMethod class, there was quite a bit of duplication there.
This patch comes courtesy of the wifi connection in my local laundry-mat here in Milwaukee. Its strange the things people will do to make their clothes wash more quickly :-).
Cheers, Adrian.
--
This message is automatically generated by JIRA.
If you think it was sent incorrectly, please contact your JIRA administrators: https://issues.apache.org/jira/secure/ContactAdministrators!default.jspa
For more information on JIRA, see: http://www.atlassian.com/software/jira
[jira] [Commented] (OGNL-31) [PATCH] Some CPD fixes around
ASTMethod and ASTStaticMethod.
Posted by "Hudson (Commented) (JIRA)" <ji...@apache.org>.
[ https://issues.apache.org/jira/browse/OGNL-31?page=com.atlassian.jira.plugin.system.issuetabpanels:comment-tabpanel&focusedCommentId=13138555#comment-13138555 ]
Hudson commented on OGNL-31:
----------------------------
Integrated in ognl #169 (See [https://builds.apache.org/job/ognl/169/])
OGNL-31 - Added compiler performance test
Added OGNL-31 to the change list
OGNL-31 - Some CPD fixes around ASTMethod and ASTStaticMethod (contributed by Adrian Cumiskey)
mcucchiara : http://svn.apache.org/viewvc/?view=rev&rev=1190443
Files :
* /commons/proper/ognl/trunk/src/benchmarks/java/org/apache/commons/ognl/performance/BasePerformanceTest.java
* /commons/proper/ognl/trunk/src/benchmarks/java/org/apache/commons/ognl/performance/invocation/CompilerInvocation.java
* /commons/proper/ognl/trunk/src/benchmarks/java/org/apache/commons/ognl/performance/runtime/CommonsRuntimeWrapper.java
* /commons/proper/ognl/trunk/src/benchmarks/java/org/apache/commons/ognl/performance/runtime/OldOgnlRuntimeWrapper.java
* /commons/proper/ognl/trunk/src/benchmarks/java/org/apache/commons/ognl/performance/runtime/RuntimeWrapper.java
mcucchiara : http://svn.apache.org/viewvc/?view=rev&rev=1190437
Files :
* /commons/proper/ognl/trunk/src/changes/changes.xml
mcucchiara : http://svn.apache.org/viewvc/?view=rev&rev=1190434
Files :
* /commons/proper/ognl/trunk/src/main/java/org/apache/commons/ognl/ASTMethod.java
* /commons/proper/ognl/trunk/src/main/java/org/apache/commons/ognl/ASTMethodUtil.java
* /commons/proper/ognl/trunk/src/main/java/org/apache/commons/ognl/ASTStaticMethod.java
* /commons/proper/ognl/trunk/src/main/java/org/apache/commons/ognl/EvaluationPool.java
* /commons/proper/ognl/trunk/src/main/java/org/apache/commons/ognl/OgnlRuntime.java
* /commons/proper/ognl/trunk/src/test/java/org/apache/commons/ognl/TestOgnlRuntime.java
> [PATCH] Some CPD fixes around ASTMethod and ASTStaticMethod.
> ------------------------------------------------------------
>
> Key: OGNL-31
> URL: https://issues.apache.org/jira/browse/OGNL-31
> Project: OGNL
> Issue Type: Improvement
> Reporter: Adrian Cumiskey
> Assignee: Maurizio Cucchiara
> Priority: Minor
> Attachments: patch-OGNL31-v2.txt, patch-OGNL31.txt
>
>
> I discovered a number of PMD CPD issues around ASTMethod and ASTStaticMethod class, there was quite a bit of duplication there.
> This patch comes courtesy of the wifi connection in my local laundry-mat here in Milwaukee. Its strange the things people will do to make their clothes wash more quickly :-).
> Cheers, Adrian.
--
This message is automatically generated by JIRA.
If you think it was sent incorrectly, please contact your JIRA administrators: https://issues.apache.org/jira/secure/ContactAdministrators!default.jspa
For more information on JIRA, see: http://www.atlassian.com/software/jira
[jira] [Commented] (OGNL-31) [PATCH] Some CPD fixes around
ASTMethod and ASTStaticMethod.
Posted by "Simone Tripodi (Commented) (JIRA)" <ji...@apache.org>.
[ https://issues.apache.org/jira/browse/OGNL-31?page=com.atlassian.jira.plugin.system.issuetabpanels:comment-tabpanel&focusedCommentId=13136832#comment-13136832 ]
Simone Tripodi commented on OGNL-31:
------------------------------------
We recently discussed about removing {{@author}} tags from code in commons and we agreed that before stripping them off, original authors should be contacted.
Anyway, even if they are not active, Luke & Drew are part of the OGNL committers, so it should be fine - at least for me - to keep their name on the Javadoc.
Thanks for the patch, I'll have a look at it ASAP - at least if someone else is faster. Looks like Maurizio is working hard trowing a release :)
> [PATCH] Some CPD fixes around ASTMethod and ASTStaticMethod.
> ------------------------------------------------------------
>
> Key: OGNL-31
> URL: https://issues.apache.org/jira/browse/OGNL-31
> Project: OGNL
> Issue Type: Improvement
> Reporter: Adrian Cumiskey
> Assignee: Maurizio Cucchiara
> Priority: Minor
> Attachments: patch-OGNL31.txt
>
>
> I discovered a number of PMD CPD issues around ASTMethod and ASTStaticMethod class, there was quite a bit of duplication there.
> This patch comes courtesy of the wifi connection in my local laundry-mat here in Milwaukee. Its strange the things people will do to make their clothes wash more quickly :-).
> Cheers, Adrian.
--
This message is automatically generated by JIRA.
If you think it was sent incorrectly, please contact your JIRA administrators: https://issues.apache.org/jira/secure/ContactAdministrators!default.jspa
For more information on JIRA, see: http://www.atlassian.com/software/jira
[jira] [Commented] (OGNL-31) [PATCH] Some CPD fixes around
ASTMethod and ASTStaticMethod.
Posted by "Maurizio Cucchiara (Commented) (JIRA)" <ji...@apache.org>.
[ https://issues.apache.org/jira/browse/OGNL-31?page=com.atlassian.jira.plugin.system.issuetabpanels:comment-tabpanel&focusedCommentId=13138544#comment-13138544 ]
Maurizio Cucchiara commented on OGNL-31:
----------------------------------------
{quote}
Although harmless, the compiler caching implementation wasn't ideal or necessary so I have removed it. The context is only passed to the getCompiler() method in order for the OgnlRuntime to perform its class resolving. Once this has been achieved the first time the context and class resolving is unnecessary on subsequent calls so we can just return the valid _compiler reference that is held.
{quote}
I totally agree, the compiler caching implementation was weak: running the tests I realized that it cached almost 500 different instances of the expression compiler.
I applied your patch, but I have to admit that {{getCompiler}} is not thread-safe.
After some performance tests, it'd seem that synchronization slows down the compiler getter (its instantiation doesn't cost so much).
Hence if sync is not a must in this case, I would keep out of sync this method.
What do you think guys?
> [PATCH] Some CPD fixes around ASTMethod and ASTStaticMethod.
> ------------------------------------------------------------
>
> Key: OGNL-31
> URL: https://issues.apache.org/jira/browse/OGNL-31
> Project: OGNL
> Issue Type: Improvement
> Reporter: Adrian Cumiskey
> Assignee: Maurizio Cucchiara
> Priority: Minor
> Attachments: patch-OGNL31-v2.txt, patch-OGNL31.txt
>
>
> I discovered a number of PMD CPD issues around ASTMethod and ASTStaticMethod class, there was quite a bit of duplication there.
> This patch comes courtesy of the wifi connection in my local laundry-mat here in Milwaukee. Its strange the things people will do to make their clothes wash more quickly :-).
> Cheers, Adrian.
--
This message is automatically generated by JIRA.
If you think it was sent incorrectly, please contact your JIRA administrators: https://issues.apache.org/jira/secure/ContactAdministrators!default.jspa
For more information on JIRA, see: http://www.atlassian.com/software/jira
[jira] [Updated] (OGNL-31) [PATCH] Some CPD fixes around ASTMethod
and ASTStaticMethod.
Posted by "Adrian Cumiskey (Updated) (JIRA)" <ji...@apache.org>.
[ https://issues.apache.org/jira/browse/OGNL-31?page=com.atlassian.jira.plugin.system.issuetabpanels:all-tabpanel ]
Adrian Cumiskey updated OGNL-31:
--------------------------------
Attachment: patch-OGNL31.txt
> [PATCH] Some CPD fixes around ASTMethod and ASTStaticMethod.
> ------------------------------------------------------------
>
> Key: OGNL-31
> URL: https://issues.apache.org/jira/browse/OGNL-31
> Project: OGNL
> Issue Type: Improvement
> Reporter: Adrian Cumiskey
> Priority: Minor
> Attachments: patch-OGNL31.txt
>
>
> I discovered a number of PMD CPD issues around ASTMethod and ASTStaticMethod class, there was quite a bit of duplication there.
> This patch comes courtesy of the wifi connection in my local laundry-mat here in Milwaukee. Its strange the things people will do to make their clothes wash more quickly :-).
> Cheers, Adrian.
--
This message is automatically generated by JIRA.
If you think it was sent incorrectly, please contact your JIRA administrators: https://issues.apache.org/jira/secure/ContactAdministrators!default.jspa
For more information on JIRA, see: http://www.atlassian.com/software/jira
[jira] [Commented] (OGNL-31) [PATCH] Some CPD fixes around
ASTMethod and ASTStaticMethod.
Posted by "Maurizio Cucchiara (Commented) (JIRA)" <ji...@apache.org>.
[ https://issues.apache.org/jira/browse/OGNL-31?page=com.atlassian.jira.plugin.system.issuetabpanels:comment-tabpanel&focusedCommentId=13136950#comment-13136950 ]
Maurizio Cucchiara commented on OGNL-31:
----------------------------------------
@Adrian Cumiskey
Generally speaking I have no problem to apply your patch.
What doesn't convince me is that your patch changes lightly the semantics:
Did you take into consideration that changing the context changes the compiler (though I don't know what side-effect could cause):
{code}
OgnlExpressionCompiler comp1 = OgnlRuntime.getCompiler( context );
context.put( "root2", new Root() );
OgnlExpressionCompiler comp2 = OgnlRuntime.getCompiler( context );
assertNotSame(comp2,comp1);
{code}
> [PATCH] Some CPD fixes around ASTMethod and ASTStaticMethod.
> ------------------------------------------------------------
>
> Key: OGNL-31
> URL: https://issues.apache.org/jira/browse/OGNL-31
> Project: OGNL
> Issue Type: Improvement
> Reporter: Adrian Cumiskey
> Assignee: Maurizio Cucchiara
> Priority: Minor
> Attachments: patch-OGNL31.txt
>
>
> I discovered a number of PMD CPD issues around ASTMethod and ASTStaticMethod class, there was quite a bit of duplication there.
> This patch comes courtesy of the wifi connection in my local laundry-mat here in Milwaukee. Its strange the things people will do to make their clothes wash more quickly :-).
> Cheers, Adrian.
--
This message is automatically generated by JIRA.
If you think it was sent incorrectly, please contact your JIRA administrators: https://issues.apache.org/jira/secure/ContactAdministrators!default.jspa
For more information on JIRA, see: http://www.atlassian.com/software/jira
[jira] [Updated] (OGNL-31) [PATCH] Some CPD fixes around ASTMethod
and ASTStaticMethod.
Posted by "Adrian Cumiskey (Updated) (JIRA)" <ji...@apache.org>.
[ https://issues.apache.org/jira/browse/OGNL-31?page=com.atlassian.jira.plugin.system.issuetabpanels:all-tabpanel ]
Adrian Cumiskey updated OGNL-31:
--------------------------------
Attachment: patch-OGNL31-v2.txt
@Maurizio Cucchiara
I believe you are making reference to a different patch contribution https://issues.apache.org/jira/browse/OGNL-23 which Simone applied.
I have attached a replacement patch which
I believe will address your concerns. I have also added the test case you described to the patch.
Although harmless, the compiler caching implementation wasn't ideal or necessary so I have removed it. The context is only passed to the getCompiler() method in order for the OgnlRuntime to perform its class resolving. Once this has been achieved the first time the context and class resolving is unnecessary on subsequent calls so we can just return the valid _compiler reference that is held.
@Adrian Crum, Simone Tripodi
I have removed both Luke and Drew as @authors in the newly introduced class ASTMethodUtil. I decided to provide a default private constructor as this class should never be instantiated and also made both the class and its methods package private as they will (and should) only ever be accessed by ASTMethod and ASTStaticMethod. I was slightly tempted to make ASTMethodUtil a base class but its methods have no dependencies on any of the instance variables in either ASTMethod or ASTStaticMethod so I decided to leave it as it is. Wishing you all a nice weekend.
Cheers, Adrian.
> [PATCH] Some CPD fixes around ASTMethod and ASTStaticMethod.
> ------------------------------------------------------------
>
> Key: OGNL-31
> URL: https://issues.apache.org/jira/browse/OGNL-31
> Project: OGNL
> Issue Type: Improvement
> Reporter: Adrian Cumiskey
> Assignee: Maurizio Cucchiara
> Priority: Minor
> Attachments: patch-OGNL31-v2.txt, patch-OGNL31.txt
>
>
> I discovered a number of PMD CPD issues around ASTMethod and ASTStaticMethod class, there was quite a bit of duplication there.
> This patch comes courtesy of the wifi connection in my local laundry-mat here in Milwaukee. Its strange the things people will do to make their clothes wash more quickly :-).
> Cheers, Adrian.
--
This message is automatically generated by JIRA.
If you think it was sent incorrectly, please contact your JIRA administrators: https://issues.apache.org/jira/secure/ContactAdministrators!default.jspa
For more information on JIRA, see: http://www.atlassian.com/software/jira
[jira] [Commented] (OGNL-31) [PATCH] Some CPD fixes around
ASTMethod and ASTStaticMethod.
Posted by "Simone Tripodi (Commented) (JIRA)" <ji...@apache.org>.
[ https://issues.apache.org/jira/browse/OGNL-31?page=com.atlassian.jira.plugin.system.issuetabpanels:comment-tabpanel&focusedCommentId=13136838#comment-13136838 ]
Simone Tripodi commented on OGNL-31:
------------------------------------
Uhm after had a quick look I got Adrian Crum's thought. Sorry.
I agree that authors of the Utils class - that is added in this new patch - are not Luke & Drew, even if they wrote the contained codebase, I agree on keeping them in existing classes but not in the new.
> [PATCH] Some CPD fixes around ASTMethod and ASTStaticMethod.
> ------------------------------------------------------------
>
> Key: OGNL-31
> URL: https://issues.apache.org/jira/browse/OGNL-31
> Project: OGNL
> Issue Type: Improvement
> Reporter: Adrian Cumiskey
> Assignee: Maurizio Cucchiara
> Priority: Minor
> Attachments: patch-OGNL31.txt
>
>
> I discovered a number of PMD CPD issues around ASTMethod and ASTStaticMethod class, there was quite a bit of duplication there.
> This patch comes courtesy of the wifi connection in my local laundry-mat here in Milwaukee. Its strange the things people will do to make their clothes wash more quickly :-).
> Cheers, Adrian.
--
This message is automatically generated by JIRA.
If you think it was sent incorrectly, please contact your JIRA administrators: https://issues.apache.org/jira/secure/ContactAdministrators!default.jspa
For more information on JIRA, see: http://www.atlassian.com/software/jira
[jira] [Commented] (OGNL-31) [PATCH] Some CPD fixes around
ASTMethod and ASTStaticMethod.
Posted by "Adrian Cumiskey (Commented) (JIRA)" <ji...@apache.org>.
[ https://issues.apache.org/jira/browse/OGNL-31?page=com.atlassian.jira.plugin.system.issuetabpanels:comment-tabpanel&focusedCommentId=13136784#comment-13136784 ]
Adrian Cumiskey commented on OGNL-31:
-------------------------------------
Luke Blanshard and Drew Davidson are the original authors of the code from ASTMethod and ASTStaticMethod, I simply refactored the duplications I found in their original work and relocated them to ASTMethodUtil. I don't feel it would be right to claim ownership myself just because I introduced a new class, if authorship is not attributed to them then to who?
Cheers, Adrian.
> [PATCH] Some CPD fixes around ASTMethod and ASTStaticMethod.
> ------------------------------------------------------------
>
> Key: OGNL-31
> URL: https://issues.apache.org/jira/browse/OGNL-31
> Project: OGNL
> Issue Type: Improvement
> Reporter: Adrian Cumiskey
> Priority: Minor
> Attachments: patch-OGNL31.txt
>
>
> I discovered a number of PMD CPD issues around ASTMethod and ASTStaticMethod class, there was quite a bit of duplication there.
> This patch comes courtesy of the wifi connection in my local laundry-mat here in Milwaukee. Its strange the things people will do to make their clothes wash more quickly :-).
> Cheers, Adrian.
--
This message is automatically generated by JIRA.
If you think it was sent incorrectly, please contact your JIRA administrators: https://issues.apache.org/jira/secure/ContactAdministrators!default.jspa
For more information on JIRA, see: http://www.atlassian.com/software/jira
[jira] [Commented] (OGNL-31) [PATCH] Some CPD fixes around
ASTMethod and ASTStaticMethod.
Posted by "Adrian Crum (Commented) (JIRA)" <ji...@apache.org>.
[ https://issues.apache.org/jira/browse/OGNL-31?page=com.atlassian.jira.plugin.system.issuetabpanels:comment-tabpanel&focusedCommentId=13136757#comment-13136757 ]
Adrian Crum commented on OGNL-31:
---------------------------------
You might want to change the author tags in ASTMethodUtil.java.
> [PATCH] Some CPD fixes around ASTMethod and ASTStaticMethod.
> ------------------------------------------------------------
>
> Key: OGNL-31
> URL: https://issues.apache.org/jira/browse/OGNL-31
> Project: OGNL
> Issue Type: Improvement
> Reporter: Adrian Cumiskey
> Priority: Minor
> Attachments: patch-OGNL31.txt
>
>
> I discovered a number of PMD CPD issues around ASTMethod and ASTStaticMethod class, there was quite a bit of duplication there.
> This patch comes courtesy of the wifi connection in my local laundry-mat here in Milwaukee. Its strange the things people will do to make their clothes wash more quickly :-).
> Cheers, Adrian.
--
This message is automatically generated by JIRA.
If you think it was sent incorrectly, please contact your JIRA administrators: https://issues.apache.org/jira/secure/ContactAdministrators!default.jspa
For more information on JIRA, see: http://www.atlassian.com/software/jira
[jira] [Issue Comment Edited] (OGNL-31) [PATCH] Some CPD fixes
around ASTMethod and ASTStaticMethod.
Posted by "Adrian Cumiskey (Issue Comment Edited) (JIRA)" <ji...@apache.org>.
[ https://issues.apache.org/jira/browse/OGNL-31?page=com.atlassian.jira.plugin.system.issuetabpanels:comment-tabpanel&focusedCommentId=13137989#comment-13137989 ]
Adrian Cumiskey edited comment on OGNL-31 at 10/28/11 4:22 AM:
---------------------------------------------------------------
@Maurizio Cucchiara
I believe you are making reference to a different patch contribution https://issues.apache.org/jira/browse/OGNL-23 which Simone applied.
I have attached a replacement patch which I believe will address your concerns. I have also added the test case you described to the patch.
Although harmless, the compiler caching implementation wasn't ideal or necessary so I have removed it. The context is only passed to the getCompiler() method in order for the OgnlRuntime to perform its class resolving. Once this has been achieved the first time the context and class resolving is unnecessary on subsequent calls so we can just return the valid _compiler reference that is held.
@Adrian Crum, Simone Tripodi
I have removed both Luke and Drew as @authors in the newly introduced class ASTMethodUtil. I decided to provide a default private constructor as this class should never be instantiated and also made both the class and its methods package private as they will (and should) only ever be accessed by ASTMethod and ASTStaticMethod. I was slightly tempted to make ASTMethodUtil a base class but its methods have no dependencies on any of the instance variables in either ASTMethod or ASTStaticMethod so I decided to leave it as it is. Wishing you all a nice weekend.
Cheers, Adrian.
was (Author: acumiskey):
@Maurizio Cucchiara
I believe you are making reference to a different patch contribution https://issues.apache.org/jira/browse/OGNL-23 which Simone applied.
I have attached a replacement patch which
I believe will address your concerns. I have also added the test case you described to the patch.
Although harmless, the compiler caching implementation wasn't ideal or necessary so I have removed it. The context is only passed to the getCompiler() method in order for the OgnlRuntime to perform its class resolving. Once this has been achieved the first time the context and class resolving is unnecessary on subsequent calls so we can just return the valid _compiler reference that is held.
@Adrian Crum, Simone Tripodi
I have removed both Luke and Drew as @authors in the newly introduced class ASTMethodUtil. I decided to provide a default private constructor as this class should never be instantiated and also made both the class and its methods package private as they will (and should) only ever be accessed by ASTMethod and ASTStaticMethod. I was slightly tempted to make ASTMethodUtil a base class but its methods have no dependencies on any of the instance variables in either ASTMethod or ASTStaticMethod so I decided to leave it as it is. Wishing you all a nice weekend.
Cheers, Adrian.
> [PATCH] Some CPD fixes around ASTMethod and ASTStaticMethod.
> ------------------------------------------------------------
>
> Key: OGNL-31
> URL: https://issues.apache.org/jira/browse/OGNL-31
> Project: OGNL
> Issue Type: Improvement
> Reporter: Adrian Cumiskey
> Assignee: Maurizio Cucchiara
> Priority: Minor
> Attachments: patch-OGNL31-v2.txt, patch-OGNL31.txt
>
>
> I discovered a number of PMD CPD issues around ASTMethod and ASTStaticMethod class, there was quite a bit of duplication there.
> This patch comes courtesy of the wifi connection in my local laundry-mat here in Milwaukee. Its strange the things people will do to make their clothes wash more quickly :-).
> Cheers, Adrian.
--
This message is automatically generated by JIRA.
If you think it was sent incorrectly, please contact your JIRA administrators: https://issues.apache.org/jira/secure/ContactAdministrators!default.jspa
For more information on JIRA, see: http://www.atlassian.com/software/jira
[jira] [Resolved] (OGNL-31) [PATCH] Some CPD fixes around ASTMethod
and ASTStaticMethod.
Posted by "Maurizio Cucchiara (Resolved) (JIRA)" <ji...@apache.org>.
[ https://issues.apache.org/jira/browse/OGNL-31?page=com.atlassian.jira.plugin.system.issuetabpanels:all-tabpanel ]
Maurizio Cucchiara resolved OGNL-31.
------------------------------------
Resolution: Fixed
Mark this issue as resolved and take the discussion of 3d-safety on a new one
> [PATCH] Some CPD fixes around ASTMethod and ASTStaticMethod.
> ------------------------------------------------------------
>
> Key: OGNL-31
> URL: https://issues.apache.org/jira/browse/OGNL-31
> Project: OGNL
> Issue Type: Improvement
> Reporter: Adrian Cumiskey
> Assignee: Maurizio Cucchiara
> Priority: Minor
> Attachments: patch-OGNL31-v2.txt, patch-OGNL31.txt
>
>
> I discovered a number of PMD CPD issues around ASTMethod and ASTStaticMethod class, there was quite a bit of duplication there.
> This patch comes courtesy of the wifi connection in my local laundry-mat here in Milwaukee. Its strange the things people will do to make their clothes wash more quickly :-).
> Cheers, Adrian.
--
This message is automatically generated by JIRA.
If you think it was sent incorrectly, please contact your JIRA administrators: https://issues.apache.org/jira/secure/ContactAdministrators!default.jspa
For more information on JIRA, see: http://www.atlassian.com/software/jira