LGTM! Awesome, thank you for making this change!

I will more closely look into this later. Looks good as a first step!

thanks @uschindler . There are many more steps. I don't care how we do it, if you want just push improvements to my branch or we can make more PRs.

I do think we should avoid cleaning up the grammar, just to be practical. @jdconrad is working heavily on this now and i dont want to conflict with that. From what I see, the generics syntax does not cause any of the problems the grammar has today but it will still simplify to get rid of it later.

Hi, thanks for the invitation to colaborate in this PR. I like the first step, but I am not happy with removing the "constants" and replacing them by getters with strings ("def", "int",...). I know why you did this, so I would take care of changing this:

Remove the copy constructor. I would make the inner part that holds the maps completely private and allocate one singleton instance using a completely private ctor (the instance field would be static final and also private). The maps inside do not necessarily be unmodifiable (we can do this later), but instead of the copy constructor, we should add static getters and static final constants that delegate to the private singleton instance, which is completely private. For consumers of definition clas,s they only see static final constants and some static, read-only getter methods. I will think about that this night and start to implement this tomorrow.

I made the public API of definition completely static. The INSTANCE field is temporarily still there, because I have to first remove the Definition parameter passed everywhere.

OK, I remved the definition parameter on all methods around analyze and write.

Thanks @uschindler for the cleanups here!!!! I pushed a couple minor cleanups, now that those internal-used types are constants, they are capitalized. And we don't need to pass CompilerSettings to every method in tree nodes.

I think this is good for now? I can think of quite a few followup cleanups but lets just get these simplifications in.

Thanks again for helping with cleanups!

Hi @rmuir, all fine from my side. Thanks for also cleaning up the compiler settings. This was the next on my TODO list. :-)

I think you should merge that now. If we do further improvements, those should be more focused on certain classes.

@jdconrad pinged me last night as he did his cleanup to boxed types in another branch. I pulled it in as its related: now there is no more transforms map, no more confusing Def vs def at all, no Utility stuff in whitelist, much better!

I agree we should stop now and get some of this in :)

@rmuir and @uschindler Thanks for all the help with cleaning this stuff up guys. I know it can be massively time consuming.

@uschindler An ASM question -- I noticed that for int -> Integer boxing the javac compiler will issue the instruction for Integer.valueOf, but using the GeneratorAdapter to do boxing, the instructions issued use new Integer(...), dup_x1, swap. Is this anything to worry about, should we be using Integer.valueOf here instead?

Edit: @rmuir has pointed out that I meant valueOf instead of parseInt. Oops!

+1 to fix that, that is wrong to do (i dont care about java 1.4 here).

Can we override that method in our adapter? http://websvn.ow2.org/filedetails.php?repname=asm&path=%2Ftrunk%2Fasm%2Fsrc%2Forg%2Fobjectweb%2Fasm%2Fcommons%2FGeneratorAdapter.java

I would just override this one for now: public void box(final Type type) {. We can do the right thing. Their unbox is fine.

We should not trust everything ASM is doing...

I would just override this one for now: public void box(final Type type) {. We can do the right thing. Their unbox is fine.

+1 (use the code from the svn link)

    /**
     * Generates the instructions to box the top stack value using Java 5's
     * valueOf() method. This value is replaced by its boxed equivalent on top
     * of the stack.
     * 
     * @param type
     *            the type of the top stack value.
     */
    public void valueOf(final Type type) {
        if (type.getSort() == Type.OBJECT || type.getSort() == Type.ARRAY) {
            return;
        }
        if (type == Type.VOID_TYPE) {
            push((String) null);
        } else {
            Type boxed = getBoxedType(type);
            invokeStatic(boxed, new Method("valueOf", boxed,
                    new Type[] { type }));
        }
    }
``

@uschindler Thanks for the pointing that out. I've updated the branch to use valueOf instead of box.

Can we override box to just call valueOf though? I dont like the method being there with inefficient code, waiting for something to call it.

Can we override box to just call valueOf though?

+1 to remove the trap. Then code can call box() or valueOf()

ok i am happy, trap is gone. thanks @jdconrad for discovering this trap

Thanks!

thanks guys for all the cleanup help! It is worth it here...