search

nhatminhle / cofoja

Contracts for Java
GNU Lesser General Public License v3.0
151 stars 18 forks source link

Re-enable openjdk javac option passthrough. #43

Closed cushon closed 9 years ago

cushon commented 9 years ago

The new implementation degrades gracefully if openjdk javac isn't available.

nhatminhle commented 9 years ago

Thanks for the patch; I've reviewed your code and put a few (mostly stylistic) comments; could you take a look and resubmit a new patch maybe? I'd be glad to merge it.

Also, it seems your implementation works also with older versions of OpenJDK; the previous implementation was retired because it only worked with OpenJDK<8 and I thought I couldn't get one that runs on both 6, 7 and 8. But I've tried yours and it seems fine, so it's all good!

cushon commented 9 years ago

Thanks for the review. I made all of the suggested changes except for the one about static initialization, which I left a question about.

Also, it seems your implementation works also with older versions of OpenJDK; the previous implementation was retired because it only worked with OpenJDK<8 and I thought I couldn't get one that runs on both 6, 7 and 8. But I've tried yours and it seems fine, so it's all good!

The previous version used the OptionName enum, which got renamed in JDK 8. Here I'm just using the string names of the options.

nhatminhle commented 9 years ago

About init() vs static constructors: actually init() is only called once per annotation processor. It's a singleton object that is constructed once by the calling tool; init() is called only once, and the same object is reused for all rounds of annotation. So there's really no overhead involved.

I think the code should live in init() because as far as I understand, it's supposed to be called once the tool has finished its own initialization and is ready to run any annotation processing. Although very likely, I think there is no guarantee that all javac classes would be loaded before our static constructor. In any case, it's better not to have to worry about class loading order at all.

cushon commented 9 years ago

Sounds good. I moved the setup to init(), and made the cached classes/methods non-static (since the processor is a singleton anyways).

nhatminhle commented 9 years ago

Oops, I forgot that two-argument exception constructors are not Java6-compatible so I patched over your commit to remove them again; sorry I told you to use them in the first place.