Bug 338118 - [compiler] CastExpression type should be changed to be a type reference and not an expression
Summary: [compiler] CastExpression type should be changed to be a type reference and n...
Status: VERIFIED FIXED
Alias: None
Product: JDT
Classification: Eclipse Project
Component: Core (show other bugs)
Version: 3.7   Edit
Hardware: PC Windows 7
: P3 normal (vote)
Target Milestone: 3.7 M6   Edit
Assignee: Olivier Thomann CLA
QA Contact:
URL:
Whiteboard:
Keywords:
Depends on:
Blocks:
 
Reported: 2011-02-24 11:36 EST by Olivier Thomann CLA
Modified: 2011-03-08 10:15 EST (History)
3 users (show)

See Also:
stephan.herrmann: review+


Attachments
Possible fix (22.97 KB, patch)
2011-02-24 14:23 EST, Olivier Thomann CLA
no flags Details | Diff
Proposed fix (23.94 KB, patch)
2011-02-24 15:02 EST, Olivier Thomann CLA
no flags Details | Diff

Note You need to log in before you can comment on or make changes to this bug.
Description Olivier Thomann CLA 2011-02-24 11:36:51 EST
By construction, the type is always a type reference. So we should make this clear inside the CastExpression type itself so that we can actually remove a bunch of instanceof checks and clean up some code.
Comment 1 Stephan Herrmann CLA 2011-02-24 13:27:19 EST
That cleanup would be great, but in my understanding the parser 
can NOT guarantee that the type is actually a type 
(that's how I read consumeCastExpressionLL1()).

Are you thinking of making the grammar smarter or of converting the expression
into a type reference inside CastExpression(Expression,Expression) ?

Excuse my curiosity :)
Comment 2 Olivier Thomann CLA 2011-02-24 13:45:05 EST
This is what I thought initially as well, but in order to get into consumeCastExpressionLL1(), it means that the rule InsideCastExpressionLL1 was called.
And this rule calls:
org.eclipse.jdt.internal.compiler.parser.Parser.consumeInsideCastExpressionLL1()
where the name is actually converted to a type reference.

So I believe that the parser can make sure that it is always a type reference. So there is no need to change the existing grammar.
Comment 3 Stephan Herrmann CLA 2011-02-24 14:10:33 EST
(In reply to comment #2)

cool!
Comment 4 Olivier Thomann CLA 2011-02-24 14:23:50 EST
Created attachment 189730 [details]
Possible fix

First draft.
Comment 5 Olivier Thomann CLA 2011-02-24 14:30:08 EST
Another good reason to fix this is that this is actually mandatory to support jsr 308 (annotations on type).
Srikanth, I let you review the patch.
Thanks.
Comment 6 Olivier Thomann CLA 2011-02-24 15:02:24 EST
Created attachment 189740 [details]
Proposed fix

Same as before with some additional cleanup.
Comment 7 Srikanth Sankaran CLA 2011-02-25 00:24:19 EST
Olivier, is this the same issue discussed here:
http://dev.eclipse.org/mhonarc/lists/jdt-core-dev/msg01909.html
or a different one ?
Comment 8 Srikanth Sankaran CLA 2011-02-25 00:26:18 EST
May be not, as that issue was resolved under https://bugs.eclipse.org/bugs/show_bug.cgi?id=292364.

I'll take a look.
Comment 9 Olivier Thomann CLA 2011-02-25 08:48:07 EST
(In reply to comment #7)
> Olivier, is this the same issue discussed here:
> http://dev.eclipse.org/mhonarc/lists/jdt-core-dev/msg01909.html
> or a different one ?
yes, but to the difference that the fix for bug 292364 didn't go as far as this one does which means that the corresponding field inside the CastExpression class should be a type refererence and not a name reference.
Comment 10 Olivier Thomann CLA 2011-02-28 13:28:42 EST
Released for 3.7M6.
Srikanth, this will be verified in the verification phase.
Comment 11 Stephan Herrmann CLA 2011-03-08 09:48:45 EST
For the records:

For a minute I was puzzled about the additional casts to TypeReference
inside CompletionParser. It turns out these are safe based on the assumption
that consumeInsideCastExpression() has been called before, which in the
case of the CompletionParser indeed pushes a type reference onto the
expression stack.
Comment 12 Stephan Herrmann CLA 2011-03-08 09:50:27 EST
Verified for 3.7 M6 via code inspection.