Review request: update javac to properly output mandated parameters in MethodParameters attributes (original) (raw)

Eric McCorkle eric.mccorkle at oracle.com
Tue Feb 12 10:01:10 PST 2013

Thanks for the reviews. I'll let you know if/when the test run makes it.

On 02/12/13 12:49, Maurizio Cimadamore wrote:

Nice! It's good to go for me.

Maurizio On 12/02/13 17:42, Eric McCorkle wrote: On 02/12/13 11:56, Maurizio Cimadamore wrote:

On 12/02/13 16:44, Eric McCorkle wrote:

Actually, it is necessary. There are specific cases where a parameter must be marked MANDATED, and it is marked SYNTHETIC in all others.

Most notably, only the outer this in a non-private inner member class constructor gets marked MANDATED. If there's an outer this for a static or a private inner class constructor, it gets marked SYNTHETIC. Well, for a static inner class the method won't be called - but for a private inner it would (I must admit that I don't understand fully the rationale as to why private constructors are omitted from the mandated thing). The choice seems somewhat arbitrary, but I have to do what the spec says... Anyway - it seems like you only need to check as to whether the owner of the method is private or not. Since this is a pretty short check, it can be inlined in the outerThisDef method. I did something different. I took the common codepaths and moved them out to private methods, and made the old outerThisDef take a ClassSymbol argument. Rationale: The MethodSymbol variant needs a MethodSymbol, not a Symbol to do what it needs to do. It needs it in two separate places: to figure out the flags, and to add to extraParameters. Doing this inline in the same function leads to two separate if-instanceof-casts, and generally uglies things up. The newest webrev has the changes. However, given the amount of messing with these codepaths that's gone on, I've submitted another JPRT run. Maurizio On 02/12/13 11:34, Maurizio Cimadamore wrote: On 12/02/13 16:22, Maurizio Cimadamore wrote: On 12/02/13 16:17, Eric McCorkle wrote: Maybe make the one for ClassSymbol take a ClassSymbol as an argument as opposed to a Symbol? Why do we need to replicate all that code? Also, I'm pretty sure that you don't need outerThisIsMandated anymore, since outerThisDef is explicitly called to add an extra 'this' argument to the constructor of an inner class.

Maurizio Maurizio On 02/12/13 10:52, Maurizio Cimadamore wrote: On 12/02/13 14:53, Eric McCorkle wrote: No, actually, that variant has to be there in order to add the outer this field in a ClassSymbol. The overloading on Symbol/MethodSymbol is very hard to parse - i.e. it's hard to tell whether a client would call one or the other. What I'm suggesting is that, since both methods seem to share 99% of the code, you add the extra logic to the old method guarded by a check (which I think is sym.kind == MTH - i.e. if the symbl is a method).

Maurizio -------------- next part -------------- A non-text attachment was scrubbed... Name: eric_mccorkle.vcf Type: text/x-vcard Size: 314 bytes Desc: not available Url : http://mail.openjdk.java.net/pipermail/compiler-dev/attachments/20130212/f3f63bb9/eric_mccorkle.vcf

More information about the compiler-dev mailing list