Community
Participate
Working Groups
Andy wants a patch format + a bugzilla for @AJ work due to some funny license issue. Here it is as drafted
Created attachment 17429 [details] @AJ draft @AJ with - non final package names - //ALEX markers all around to spot hooks - singleton aspect support final (except names etc) - LTW Java 5 drafted - LTW xml drafted - test case not fully integrated in the XML infra.
Created attachment 17463 [details] full @AJ draft since the patch does not contain new files.. per Andy request
I've started by taking the modifications to 'runtime' which: 1. Introduce EnclosingStaticPart as minimal subtype of StaticPart 2. Provides support for proceed() on a join point. (supported by joinpoint objects being aware of an around closure instance) Alex - did you see Adrians notes at: http://dev.eclipse.org/viewcvs/indextech.cgi/~checkout~/aspectj-home/doc/ajdk15notebook/ataspectj-pcadvice.html which suggest the creation of a new sub-type of JoinPoint called ProceedingJoinPoint that has the proceed method on it?
I am not in favor of the ProceedingJoinPoint but more in favor of a NO-OP when proceed() is used within before / after advices. It makes the advice signatures symetric. The programming model seems simpler as well for the user. Aside the impact on the code is reduced, since code style aspects advice invocation are unchanged. On the opposite, we would have to create this proceedingJP as soon as we see we have an around advice on a shadow no matter if it is a code style or @style advice. That is for sure something to iterate on, as well as the underlying closure/joinpoint run/proceed linkage. I just think this is enough for the RC1 we aim at demo at AOSD.
bcel-builder module. Change to MethodGen committed (and lib/bcel.jar rebuilt), now returns empty string array instead of null if no arg names set.
95% of the patch is applied to my private workspace - I've mainly stayed away from changes to LTW related stuff. I have committed a few minor changes that are fine standalone. All the testcases are still passing for me - the next thing I want to do is get Alex's single testcase to pass. Once that is passing too, I will commit - Alex can do a sync and see what I messed up ;) There are a few things here and there where I have some questions, I'll try and pull these into a sensible list ...
Hi Alex, singleton test is passing and my own two simple tests are also passing. I do have a few questions, and the best way to tackle these is likely to have a call between you and us to talk through them. But its worth listing the questions here that we want to discuss... For some of the simpler changes, we probably need to create more tests, even things like the addition of EnclosingStaticPart deserve a few unit tests to verify they work ok. JoinPoint is our public interface, it doesn't seem right to have set$aroundClosure() on it - we need to fix that (but I think you already know this isnt quite right). AroundClosure - better to have getter for relevant state than make state public. - What problem does getJoinpoint() solve? Is it used from the weaveAroundClosure(), I'm just not sure why we haven't needed it before, is the joinpoint not accessible as a parameter? SingletonAspectBindingsTest is a good test, but it probably needs some simpler ones around it, I've added a couple to start with. It's just for when they go wrong (like the around advice did this morning). Whats stored in an aj$class field - should it be ajc$class as ajc$ is our recognized prefix? Aj5Attributes - I'm worried about the performance of making multiple passes over the attributes. A first way to address it is to have a flag, set or returned by the unpackAjAttributes() method that informs the caller of whether there are any annotations worth looking at, if not we can avoid making that second pass. But this still leaves us with the problem of pointcut annotations, and going through all the methods as part of unpacking the annotations on the class in order to find them - we need to think about the best way to solve this... Need to move the hardcoded strings to constants somewhere...that tripped me up a couple of times. Ought to rename from Aj5Attributes to ATAttributes or something, i'm not sure what. CastExpr/FieldGetOn/StringConstExpr - could do with a bit more doc. I admit we are lacking in doc in loooooooads of places but we are trying to change that as we move forward. Commenting out the munger sorting in BcelWeaver may help the @Aj syntax but breaks some testcases - it would be better to determine an appropriate key for the sort other than the name. Given the nature of the changes, we've been discussing the best way to share this code amongst the team - we think the best way will be a CVS branch in which these features can be finished, then we can merge them into HEAD. One good reason for this is that the build infrastructure is currently completely unaware of Java5 and it'll take me a little while to ensure aspectjrt.jar is built appropriately and the 1.5 tests are executed correctly. what do you think? I'll attach my current version of the patch to this bug report - just so it isnt only on my machine !
Created attachment 17506 [details] Zip of 4 patches to 4 modules and new 'java5' module. This file contains patches to org.aspectj.ajdt.core/tests/weaver/runtime and a zip of the new module 'java5'. The patches were generated in eclipse and do include new files (e.g. the new tests). Its quite amazing to see annotation syntax programs compile and run :) This patch includes the minimum from the first patch to get annotation syntax to work.
Yes a branch is a good idea. Can you do one now, so that we iterate in it tomorrow ? The strategy for the branch will be to reduce the changes in the existing base to avoid having conflicts during the merge later on. As I said, this patch is a draft (hence all those strings, public access instead of accessors etc). That was the best way to point f.e. this build issue for Java 5 in the current system, recently fixed enum as local variable naming issue , packaging issues, etc. The around advice closure/jp will be refactored as time goes. The short explanation for now is that - for code style, the relation is closure-->jp [closure has jp in its state] - for @ style, the relation is jp-->closure [jp.proceed() must reach closure.run()] The munger sorting cannot be solved. There must be NO ordering since it would actually un-order the source code ordering. The source code ordering for code style aspect should rely on the compiler (just as for javac) and not on name ordering - even if so far the advice name where indeed in the correct order - something that cannnot be controlled with @AJ aspects. I ll add tests and fix those issues in the branch. Optimizations way later as well. Let me know what's the branch name.
Note on joinpoint.proceed() If we plan to have a proceed(Object..) varargs based version, then we will need a ProceedingJoinPoint (extends JoinPoint) since that one will be Java 5 specific. When we then have a 1.4 port, we will have to have proceed(Object[]) and then we d better not tied us to varargs. In 1.4 the user will do the boxing as needed. It thus seems proceed should belong to the 1.4 world. Be it JoinPoint with a NO-OP strategy for before/after or be it ProceedingJoinPoint. Note: @AJ proceed(...) is not yet impl. Only proceed() for now.
closing was a generic @AJ one for several things. We agreed on ProceedingJoinPoint, and the other things have been handled during the branch to head merge.
