Bugzilla – Full Text Bug Listing
| Summary: | Suspicious fall-through | ||||||
|---|---|---|---|---|---|---|---|
| Product: | [Eclipse Project] JDT | Reporter: | Dani Megert <daniel_megert> | ||||
| Component: | APT | Assignee: | Walter Harley <eclipse> | ||||
| Status: | VERIFIED FIXED | QA Contact: | |||||
| Severity: | normal | ||||||
| Priority: | P3 | CC: | eclipse, jarthana, Olivier_Thomann | ||||
| Version: | 3.6 | Flags: | daniel_megert:
review+
|
||||
| Target Milestone: | 3.6.1 | ||||||
| Hardware: | PC | ||||||
| OS: | Windows XP | ||||||
| Whiteboard: | |||||||
| Attachments: |
|
||||||
|
Description
Dani Megert
Agree. Code like that says one thing to me: there is no test covering this case. In lieu of a specific bug from the field I am reluctant to make any code change in 3.6 at this point. But this definitely needs a test case and most likely a fix in 3.6.1. Dani, do you agree, or do you think we should try to get a fix into 3.6? I agree. This is clearly boggus and a break; statement should be added with a regression test. It is clear that throwing an exception is not expected there. I don't think this should wait as the fix is trivial and low risk. Theoretically someone could catch the exception. There's just no benefit to touch it at this point. (In reply to comment #4) > Theoretically someone could catch the exception. There's just no benefit to > touch it at this point. If someone catches this exception, he/she should probably not be a software developer :-). This being said, the benefit is to prevent a bug report from being open in the future once someone will actually use that code. But it is your call. Note that the compiler properly detects that case if the fall-through warning is enabled. I am with Dani on this. The code has been like that for quite a few years now and no one seems to have hit it (which is itself quite interesting). I do not think I could argue the case for needing a code change in RC2 on that basis, and I have been bit a few too many times by trivial safe changes at the last minute :-) It will be interesting to see what happens when I try to write a regression test. I wonder if it is actually dead code. It is hard to believe it could have been in there so long otherwise. Have been working on it. The fix is indeed trivial and doesn't seem to break anything. I'm working on adding tests, which is a pain because the old apt.tests code is so random. I believe the reason no one has reported running into it is that it's an unusual operation: asking for the source position of an annotation member _declaration_. Usually one wants a source position in order to report an error, and you'd typically report an error against the annotation instance, not its declaration. But anyway, still looks good to fix in 3.6.1. Created attachment 173494 [details]
Proposed patch and test case
This should be fixed in 3.6.1 and in 3.7.
Dani or Olivier, can you approve for 3.6.1?
+1 for 3.6.1. I only reviewed the product code change. Olivier, can you review the test changes? Thanks. Fixed in HEAD and 3.6.1. I have also branched and released the affected projects. I went ahead and committed the changes without Olivier's test review, because that would only affect the tests. Olivier, please reopen the bug if you would like me to change anything. > I have also branched and released the affected projects.
Why did you branch 'org.eclipse.jdt.apt.ui'? The rule is that we only branch a project when it has a change but I could not see any change in there.
Verified for 3.6.1 RC2 by code inspection and regression tests. |