Bug 320425

Comment 1 Kris De Volder 2010-07-21 13:58:04 EDT

Created attachment 174891 [details]
partial fix, for discussion / review

I'm attaching a patch with a partial fix.

Currently stuck because the AspectJ side has different handles, and they do not contain enough information to know whether the intertype element is field or method.

So we probably need to get a similar change in handle structure in the AspectJ side.

Comment 2 Andrew Eisenberg 2010-07-21 15:06:48 EDT

> So we probably need to get a similar change in handle structure in the AspectJ
> side.
Yes.  We will need a corresponding change in AspectJ.

Some comments on the patch:

1. IntertypeElement should not implement IField.  FieldIntertypeElement should.  This was the change that originally kicked off this refactoring.

2. As we discussed earlier, I would prefer to rejigger the handle identifier characters, rather than create a dummy parameter for ITDs to distinguish between fields and methods.

Here is my proposal for new handle characters:

Existing:

JEM_ASPECT_CU = '*'
JEM_ADVICE = '&'
JEM_ASPECT_TYPE = '}'
JEM_CODEELEMENT = '?'
JEM_ITD = ')'
JEM_DECLARE = '`'
JEM_POINTCUT = '+'

New (changes only):

JEM_ITD = (removed)
JEM_ITD_METHOD = ')'
JEM_ITD_FIELD = ','
JEM_POINTCUT = '"'
JEM_ASPECT_TYPE = '''

This ensures that we avoid all conflicts with org.eclipse.jdt.core.Signature and all other existing JEM characters.

Comment 3 Andrew Eisenberg 2010-07-21 15:08:14 EDT

Created attachment 174901 [details]
File from kdvolder describing existing possible characters

Attaching a file sent to me by kdvolder.  This file describes all possible ASCII chars we can use as well as the ones currently in use.

Comment 4 Kris De Volder 2010-07-21 15:23:44 EDT

OK, I'll have a go at implementing this.

Comment 5 Andrew Clement 2010-07-21 17:31:21 EDT

so... what do you want changing in AJ?

Comment 6 Kris De Volder 2010-07-22 16:09:09 EDT

Created attachment 175021 [details]
Patch adressing the AJDT side of the issue

This patch is a fix for the AJDT side of things. 

I'll be working on a list with some examples of handles of what we want changed on the AspectJ side. 

After the AspectJ side is changed some of the code in this patch will be obsolete, so should be revisited.

Issue should stay open until this is done.

Comment 7 Kris De Volder 2010-07-22 16:50:03 EDT

Hi Andy, here is the promised summary, with some examples, of what we want changed in the AspectJ compiler's handle identifiers. Let me know if you need more info.

1) JEM_ASPECT_TYPE: the character that indicates ab "aspect type" entity

  old: '}'
  new: '\''

Example handle: 
  GetInfo aspect 
         in file GetInfo.aj
         in package src
         in project TJP Example
  old: "=TJP Example/src<tjp{GetInfo.aj}GetInfo"
  new: "=TJP Example/src<tjp{GetInfo.aj'GetInfo"

2) JEM_ITD: the character that indicates an "ITD" element
  old: ')'
  new for method / constructor :  ')'  i.e. unchanged
  new for field                :  ','
  
The handles for ITD method and constructor are not affected by this change
only the handles for ITD fields are changed.

example handle: 
  Point.support intertype field
    in aspect BoundPoint
    in file BoundPoint.aj
    in package bean
    in folder src
    in project Bean Example

  old: "=Bean Example/src<bean{BoundPoint.aj}BoundPoint)Point.support" 
  new: "=Bean Example/src<bean{BoundPoint.aj'BoundPoint,Point.support" 

3) JEM_POINTCUT
  old: '+'
  new: '"'

example handle
  named pointcut "helmCommandsCut" with parameter type "Ship"
    in class "Ship"
    in file "Ship.aj"
    ...

  old: "=Spacewar Example/src<spacewar{Ship.aj[Ship+helmCommandsCut+QShip;"
  new: "=Spacewar Example/src<spacewar{Ship.aj[Ship\"helmCommandsCut\"QShip;"

Comment 8 Andrew Eisenberg 2010-07-22 18:15:28 EDT

Thanks for the patch.  I have applied it.  It is mostly looking good to me.  I made a few changes:

1. AJMementoTokenizer should have uncommented the bits about JEM_ANNOTATION because there is no longer a conflict with it.

2. I converted IntertypeElemen.createMockElement to being abstract and implement it in the subtypes.

Comment 9 Andrew Eisenberg 2010-07-22 18:18:37 EDT

Comment on attachment 174901 [details]
File from kdvolder describing existing possible characters

Adding iplog for kdvolder.  File was uploaded by me, but originally created by kdvolder.

Comment 10 Andrew Eisenberg 2010-07-22 18:19:23 EDT

Patch is applied and committed.  Build server is in a state of disarray and tests are not running.  I will wait until a successful build before closing.

Comment 11 Andrew Clement 2010-07-22 20:47:28 EDT

New AspectJ jars are now in AJDT with the changes,i think.

Comment 12 Kris De Volder 2010-07-22 23:10:35 EDT

Created attachment 175038 [details]
Patch to remove obsolete handle mapping code

Now that AspectJ compiler has been fixed by Andy to be in line with changes in AJDT handles...

We must remove the provisional mapping code.

Attached patch does that. This patch should be applied ASAP, because without it a number of tests will fail.

Comment 13 Andrew Eisenberg 2010-07-23 00:36:20 EDT

Kris's final patch is committed.  Thanks for responding so fast.

Comment 14 Andrew Eisenberg 2010-07-23 18:43:11 EDT

Resolving this bug.  There may be some issues that the patches used here opens up, but I will raise new bugs for them.