Fix for issue 188

Fix for issue 188: error-prone fails when compiling something that uses an annotation processor.

[cushon, 2014-01-08 18:35:32]

Sorry for the slow review. Looks good overall.

https://codereview.appspot.com/37830044/diff/1/core/src/main/java/com/google/...
File core/src/main/java/com/google/errorprone/ErrorProneCompiler.java (right):

https://codereview.appspot.com/37830044/diff/1/core/src/main/java/com/google/...
core/src/main/java/com/google/errorprone/ErrorProneCompiler.java:128:
context.put(Scanner.class, errorProneScanner);
Is it safe to remove the check that the context doesn't already contain a
scanner?

[eaftan, 2014-01-10 20:22:27]

https://codereview.appspot.com/37830044/diff/1/core/src/main/java/com/google/...
File core/src/main/java/com/google/errorprone/ErrorProneCompiler.java (right):

https://codereview.appspot.com/37830044/diff/1/core/src/main/java/com/google/...
core/src/main/java/com/google/errorprone/ErrorProneCompiler.java:128:
context.put(Scanner.class, errorProneScanner);
On 2014/01/08 18:35:33, cushon wrote:
> Is it safe to remove the check that the context doesn't already contain a
> scanner?

I originally thought so, but now I think maybe not.  I moved that check for
whether the Context already contains a Scanner to ErrorReportingJavaCompiler,
and I thought that would always execute before this. But looking more closely
that doesn't always seem to be true.  I'll add that check back in as an || with
what I have there currently.

[cushon, 2014-01-10 22:44:11]

> I originally thought so, but now I think maybe not.  I moved that check for
> whether the Context already contains a Scanner to ErrorReportingJavaCompiler,
> and I thought that would always execute before this. But looking more closely
> that doesn't always seem to be true.  I'll add that check back in as an ||
with
> what I have there currently.

Context.put(...) throws an exception if the same key is added twice. Shouldn't
we be checking that a scanner hasn't been added even when errorProneScanner !=
null?

[eaftan, 2014-01-11 01:31:56]

You're right.  Fixed in rev 7ea45aa92ec6.


On Fri, Jan 10, 2014 at 2:44 PM, <cushon@google.com> wrote:

> I originally thought so, but now I think maybe not.  I moved that
>>
> check for
>
>> whether the Context already contains a Scanner to
>>
> ErrorReportingJavaCompiler,
>
>> and I thought that would always execute before this. But looking more
>>
> closely
>
>> that doesn't always seem to be true.  I'll add that check back in as
>>
> an || with
>
>> what I have there currently.
>>
>
> Context.put(...) throws an exception if the same key is added twice.
> Shouldn't we be checking that a scanner hasn't been added even when
> errorProneScanner != null?
>
> https://codereview.appspot.com/37830044/
>