Bug 340022

Comment 1 Satyam Kandula 2011-03-16 05:43:07 EDT

Created attachment 191291 [details]
Proposed patch

Comment 2 Satyam Kandula 2011-03-16 05:54:34 EDT

Olivier, Srikanth, 
I am releasing the patch. However, I want you to have a look at the changes and give me feedback.

Comment 3 Satyam Kandula 2011-03-16 06:04:34 EDT

Released on BETA_JAVA7

Comment 4 Srikanth Sankaran 2011-03-19 08:13:03 EDT

(In reply to comment #2)
> Olivier, Srikanth, 
> I am releasing the patch. However, I want you to have a look at the changes and
> give me feedback.

(1) IsEffectivelyFinal is using Bit21 - why ? Looks like for local variable
bindings, the lowest number used is 11 IsArgument. We may have to renumber
MultiCatchParameter and IsResource too ? 

(2) Bring the declaration of IsEffectively final under the
// java 7 - multi catch parameter --- local
comment

(3) The following files look ok:

Assignment.java
CompoundAssignment.java
LocalVariableBinding.java
TagBits.java
VariableBinding.java
ComplianceDiagnoseTest.java
TryStatement17Test.java

(4) W.r.t the following files:

TryStatement.java
ExceptionHandlingFlowContext.java
FlowContext.java 

I would like to sit with you to discuss. At the outset it looks like
these changes could be significantly simplified: for example, after
translating the throw argument into an array of precise exceptions in
org.eclipse.jdt.internal.compiler.flow.FlowContext.checkExceptionHandlers(TypeBinding, ASTNode, FlowInfo, BlockScope)

why won't we simply invoke the other method i.e., 

org.eclipse.jdt.internal.compiler.flow.FlowContext.checkExceptionHandlers(TypeBinding[], ASTNode, FlowInfo, BlockScope) ?

This would be help retain the former as the lighter version it is intended
to be as well as keep the code clutter to a minimum. (Unless I overlooked
something)

Similarly, I think the rest of the changes could be significantly simplified
if the set of precise types caught is an attribute of a catch block itself
instead of the exception handling flow context - better yet, if we would
introduce a new binding type say CatchParameterBinding that is a subtype of
local variable binding then that could have an attribute preciseTypes [].

bug 340485 and bug 340484 raised as follow ups, I'll continue to test
some more.

Comment 5 Srikanth Sankaran 2011-03-20 02:56:25 EDT

Created attachment 191571 [details]
A modified patch that simplifies somethings

This patch retains most of the earlier patch, but
simplifies the changes in TryStatement.java, FlowContext.java
and ExceptionHandlingFlowContext.java.

Satyam, please review and let us discuss whether it makes sense
to use this simpler version.

For review please use predecessor for all elements other than
the following:

For FlowContext.java  - compare with 1.68.2.2
For TryStatement.java - compare with 1.116.2.13
For ExceptionHandlingFlowContext - compare with 1.45.8.1

Passes all tests.

This patch also fixes the bug 340485 and bug 340484 and
contains junits for them

Comment 6 Srikanth Sankaran 2011-03-20 03:01:17 EDT

(In reply to comment #5)
> Created attachment 191571 [details]
> A modified patch that simplifies somethings

> For review please use predecessor for all elements other than
> the following:
> 
> For FlowContext.java  - compare with 1.68.2.2
> For TryStatement.java - compare with 1.116.2.13
> For ExceptionHandlingFlowContext - compare with 1.45.8.1

Don't compare with predecessor for these elements as it can
be very confusing. Comparing with the version specified should
make the review a breeze.

Comment 7 Srikanth Sankaran 2011-03-21 02:40:45 EDT

Created attachment 191594 [details]
Modified patch that simplifies somethings

(In reply to comment #5)
> Created attachment 191571 [details]
> A modified patch that simplifies somethings
> 
> This patch retains most of the earlier patch, but

I overlooked some changes and didn't retain them, This
latest patch fixes that issue.

Comment 8 Srikanth Sankaran 2011-03-21 04:52:43 EDT

Created attachment 191596 [details]
Final patch

This patch includes some more of the original patch's code
that was overlooked by mistake.

Satyam and I reviewed and discussed this patch and decided
it is useful to go for the simplification it offers.

Passes all tests. Released in BETA_JAVA7 branch.

Next steps:

(1) Satyam, please mark the review flag.
(2) We need to add more tests, particularly runtime tests
for multi catch as well as data flow related tests.

Comment 9 Satyam Kandula 2011-03-21 07:19:56 EDT

I have reviewed the changes and the changes look good. I will add more tests.

Comment 10 Ayushman Jain 2011-06-29 10:00:35 EDT

Verified using Eclipse Java 7 Support(Beta) feature patch v20110623-0900.