Bug 320841

Summary:

TypeConverters don't set enclosingType

Product:

[Eclipse Project] JDT

Reporter:

Stephan Herrmann <stephan.herrmann>

Component:

Core

Assignee:

Jay Arthanareeswaran <jarthana>

Status:

VERIFIED FIXED

QA Contact:

Severity:

minor

Priority:

CC:

amj87.iitr, daniel_megert, satyam.kandula, srikanth_sankaran

Version:

3.7

Target Milestone:

3.7 M1

Hardware:

OS:

Linux

Whiteboard:

Attachments:

Description	Flags
proposed patch	jarthana: iplog+

Description Stephan Herrmann

2010-07-25 07:49:32 EDT

If been wondering for some years :) why the TypeConverters don't set
enclosingType on member type declarations.

Only today I found an additional inconsistency in this area, but I should
say that I haven't yet found a test case where this actually matters.
I started to look into this because I found the reconciler reporting
bogus errors, which actually never show up at the UI.

For Object Teams I would like to rely on the enclosingType field to
always be set for member types. Does that seem to be a sound assumption?

Comment 1 Stephan Herrmann

2010-07-25 08:32:27 EDT

Created attachment 175173 [details]
proposed patch

This patch adds a quite obvious line to both SourceTypeConverter and 
BinaryTypeConverter:
  typeDeclaration.memberTypes[i].enclosingType = typeDeclaration;
If this was omitted on purpose, I'd love to learn why.

The other inconsistency I was seeing was actually false alarm:
For a while I was thinking that also SourceElementNotifier should be changed
to pass the current type as declaringType into    notifySourceElementRequestor(TypeDeclaration,boolean,TypeDeclaration,ImportReference)
(Call in line 678).

However, on debugging this I see the difference between the declaringType
of a local type and the enclosingType of a member type. Here I simply suggest 
to rename the parameter to avoid confusion. No big deal.

Comment 2 Srikanth Sankaran

2010-07-26 10:23:35 EDT

Jay, please take it forward. TIA.

Comment 3 Jay Arthanareeswaran

2010-07-27 06:16:29 EDT

Patch looks good. I will release later today.

About the declaringType parameter, I don't seem to run into a case where a non-null is passed for declaringType. I mean, if the declaringType is a declaring type of a local type, won't it be covered as field's declaring type? Besides for consistency case (we use enclosing type for outer/inner types) we might want to leave it as is but with a clarifying documentation, of course.

Comment 4 Dani Megert

2010-07-29 02:28:13 EDT

Jay, I saw you already committed the patch - not good, because

- every source file that gets changes from someone that is not covered by
  the company mentioned in the copyright notice must be added like this:
name <e-mail> - bugzilla summary - bugzilla URL
e.g.
Dani Megert <dani@eclipse.org> - My bug summary - https://bugs.eclipse.org...

- the iplog+ flag must be set on the patch in the this bug

Comment 5 Jay Arthanareeswaran

2010-07-29 04:09:04 EDT

(In reply to comment #4)
> Jay, I saw you already committed the patch - not good, because
> 
> - every source file that gets changes from someone that is not covered by
>   the company mentioned in the copyright notice must be added like this:
> name <e-mail> - bugzilla summary - bugzilla URL
> e.g.
> Dani Megert <dani@eclipse.org> - My bug summary - https://bugs.eclipse.org...
> 
> - the iplog+ flag must be set on the patch in the this bug

Sorry, I thought the 'Others' in the copyright should cover this. Is it too late to add to the copyright and set the iplog+ now?

Comment 6 Dani Megert

2010-07-29 04:18:19 EDT

>Sorry, I thought the 'Others' in the copyright should cover this. 
http://www.eclipse.org/legal/copyrightandlicensenotice.php

>Is it too
>late to add to the copyright and set the iplog+ now?
No, just add the line and update the flag.

Comment 7 Jay Arthanareeswaran

2010-07-29 05:19:58 EDT

Released in HEAD for 3.7 M1.
Verification to be done by code inspection.

Comment 8 Satyam Kandula

2010-08-04 07:54:43 EDT

Verified for 3.7M1 using build I20100802-1800 looking at the source code