Community
Participate
Working Groups
I20080122-1600 Fup of bug 100278 Given: public class C { public static final class MyError extends Error {} public void foo() { try { bar(); } catch (MyError e) { } } private void bar() {} } 1. Set errors/warnings 'Innecessary declaration of thrown exception' to warning 2. disable 'Ignore unchecked exception' 3. Ok Is: compile error: Unreachable catch block for C.MyError. This exception is never thrown from the try statement body If I disable this new option I get 24 errors in Junit 3.8
Created attachment 88123 [details] Fix + test case (not fully tested yet) Had to separate the notion of 'reached' from the notion of 'clearing the warning', since we always want to consider that unchecked exceptions are reached. Tests are running.
This patch passes all (our) tests and can be used for patching, but I'll work more on a better solution before releasing.
Created attachment 88250 [details] Better fix + test cases This fix clarifies a bit the initialization of the new bits field. We may want to explore time/space optimization a bit further under various hypothesis. In terms of test coverage, the issue detected here came from the fact that there was no clear separation between the two diagnosis. This patch makes clear that only the warning can get affected, hence we do not needed to replicate all exceptions-related tests for the sake of testing them again under the new option. The original test case, reflected into ProgrammingProblemsTest#37, should hence be OK.
Digging further into this, it becomes clear that we use this context for methods and for try statements, which have different needs in terms of analysis. I will make a try at using the existing data structure only, tuning its initialization depending on the values of the couple (method or try statement, report declared unchecked or not). Only isReached must change indeed, but I also remarked an optimization opportunity around isNeeded (I believe we can save an array copy).
Created attachment 88269 [details] Fix + test case Applying the reasoning of the previous comment results into this, which is far better in terms of space, and better in terms of time. The trick is to play with methods and try statements separately. This patch passes the tests. I'll make yet another one that uses a single loop in ExceptionHandlingFlowContext constructor. (I initially preferred speed to code compactness and split the loop into two. Thinking more about it, I doubt we get so many unchecked exceptions in the list, which would means that using a boolean flag could do the trick. I would have to check that assumption yet, which I'll do in the interim.)
A rapid check on TryStatementTest gives us way more unchecked exceptions than checked exceptions (225/76), which would mean that tightening the loop would be the way to go. I will then ask Kent to tell me what he thinks of the said patch. (Philippe wanted to discuss this bug as well, and is more than welcome.)
I'll leave this one to Philippe since he wrote the code ;)
Created attachment 88410 [details] Fix + test case - factorized loops Discussed the matter with Philippe this morning and he prefers that we factorize the loops to avoid potential partial edits. Tests pass.
Philippe, this is the change we discussed this morning. Would you please review it?
Got 'live' review from Philippe, and then: Released for 3.4M5.
Verified for 3.4M5 using build I20080204-0010
