Bugzilla – Full Text Bug Listing
| Summary: | Inconsistent use of Java 5, too many warnings | ||
|---|---|---|---|
| Product: | z_Archived | Reporter: | Igor Jacy Lino Campista <icampista> |
| Component: | Mylyn | Assignee: | Holger Voormann <eclipse> |
| Status: | CLOSED FIXED | QA Contact: | David Williams <david_williams> |
| Severity: | normal | ||
| Priority: | P3 | CC: | d_a_carver, eclipse, florian |
| Version: | unspecified | ||
| Target Milestone: | --- | ||
| Hardware: | PC | ||
| OS: | Windows 7 | ||
| Whiteboard: | |||
| Attachments: | |||
|
Description
Igor Jacy Lino Campista
Created attachment 166742 [details]
Patch that fixes 549 warnings, clarifies interfaces and simplifies many lines of code.
So.... what started as a bit of updating, end up in a big update of as much as I could, leaving modified things backwards compatible, leaving untouched what I was not sure or did not make sense (left some comments in the few places(5 or so) ).
After my big update, 1938 warnings are appearing in the eclipse problems view. (299 if disabling serialization and discouraged API warnings)
So 549 warnings less. And in most cases code is reduced by half or more and of course is much more clear. Everything is compiling fine, all JUnit tests are green.
Cheers,
Igor
PS: I notice some detail of the EMF model that perhaps needs a correction. Validator.getValidRootElements() seems like it should return a Set<String>
(In reply to comment #1) Thank you for the patch. Unfortunately, it seems the patch is not against CVS head, for instance the last change of ListenerList took place 18th Februar 2010. Could you please update and contribute this patch again? Created attachment 166821 [details]
Patch that fixes 333 warnings, clarifies interfaces and simplifies many lines of code.
oh man, that was the reason of the date. :(
ok, adapted to the trunk.
The trunk had less warnings, but still got several things improved.
warnings before the patch: 442 warnings after the patch: 109 I left out tests cleaning on purpose. When the patch (333 warning fixes) will be taken, I have a 2nd patch that improves the Box interface further and also cleans some tests. +1 for the second patch, a big step towards the goal of 0 warnings (In reply to comment #6) > +1 for the second patch, a big step towards the goal of 0 warnings Due to the size of the patch, I suspect we will have to send through IP review just to be safe. Shouldn't take long for it go through the review. Holger or FTL do yo you guys want to submit the appropriate CQ? It'll be a good learning process. (In reply to comment #7) > Holger or FTL do yo you guys want to submit the appropriate CQ? It'll be a > good learning process. Yes, but what's the next step for me? Should I review it or added it to the IP log first? (In reply to comment #8) > (In reply to comment #7) > > Holger or FTL do yo you guys want to submit the appropriate CQ? It'll be a > > good learning process. > > Yes, but what's the next step for me? Should I review it or added it to the IP > log first? Need to review it, FTL and I discussed on IRC that we may want to break the patch down into smaller patches, just to make it easier to deal with. Make sure appropriate header information is updated as well, with bug number in each file that was touched. (In reply to comment #9) > Need to review it, FTL and I discussed on IRC that we may want to break the > patch down into smaller patches, just to make it easier to deal with. Make > sure appropriate header information is updated as well, with bug number in each > file that was touched. > Okay. Smaller patches (per plug-in?) would be nice. I will review the first part at Friday. I have notice two files that perhaps were the byproduct of switching to the HEAD. src/org/eclipse/wst/xml/vex/core/internal/validator/DFAState.java (-114 lines) src/org/eclipse/wst/xml/vex/core/internal/validator/DTDValidator.java (-330 lines) Looks like they were removed since December and no are no more in the HEAD but they were by some reason registered in the patch in removed state. Created attachment 167261 [details]
Patch that fixes 333 warnings, clarifies interfaces and simplifies many lines of code.
For the sake of completeness, removed the 2 file side-effect parts. File size was 14% reduced :)
Created attachment 167543 [details] Igor's patch reviewed (In reply to comment #12) Patch reviewed: looks good :-) Minor changes: * SacFactory no longer exists * DomConfigurationElement#62: return children.toArray(new IConfigElement[]{}); => return children.toArray(new IConfigElement[children.size()]); * "Association" and corresponding test deleted (seems to be dead code) The size of the patch is greater than 200 lines but less than 250 lines which have been changed or deleted: so the patch must not go through IP. As soon as Dave succeeded (http://intellectualcramps.blogspot.com/2010/05/build-stress-relief.html) in CI (https://build.eclipse.org/hudson/view/WTP/job/cbi-wtp-inc.vex/) and Chuck is happy again I will apply the patch. ;-) Igor, thank you very much for cleaning-up the code! :-))) (In reply to comment #13) Patch applied in HEAD (In reply to comment #5) > ... > When the patch (333 warning fixes) will be taken, > I have a 2nd patch... I'm leaving this bug open for the 2nd patch. Thx again for this improvement, Igor. Created attachment 167634 [details]
more use or generics and 52 warning fixes
Includes several generic usage updates, replaced some deprecated methods, Validator.rootElements signature correction and several warning fixes in test plugins (including missing localization settings).
27 warnings are missing (not including serialization or API restriction), they are very difficult to corrent since they are coming from Eclipse API not using generics or deprecated methods with replacement not clear to me. -> I think already out of the scope of this bug ;)
Created attachment 167635 [details]
more use of generics and 52 warning fixes
(In reply to comment #16) > Created an attachment (id=167635) [details] > more use of generics and 52 warning fixes 2 additional comments to the 2nd patch: 1) I don't understand why the TableLayoutTest.java has (-244 / +243 lines), there were some 6-8 line changes, but "Create Patch..." choses to include the whole file. 2) The AbstractAddRowHandler.java had a bit more refactoring to extract the concept of RowCell. I could simplify more the lines 90-98 but chose not to since I don't know if the list changes. (In reply to comment #17) > 2) The AbstractAddRowHandler.java had a bit more refactoring to extract the > concept of RowCell. I could simplify more the lines 90-98 but chose not to > since I don't know if the list changes. The refactoring of AbstractAddRowHandler is definitely more complex than the most of the other changes in the patches. Igor, could you enter an own issue with a description of what should be done, to separate this refactoring from the other, more simple changes? There should also be a unit test to ensure that the logic is not changed. Igor, you did a great job cleaning up the code base. I really like what you contributed so far. Do you want to be a committer for Vex? Created attachment 168023 [details]
2nd patch that fixes 52 warnings, more generics updates , replace some deprecated apis, plugin build/localization corrections
AbstractAddRowHandler has now basic generics update. (a separate bug will be created for the more complex refactoring :)
(In reply to comment #18) I have created a separate bug with the patch for the AbstractAddRowHandler simplification at https://bugs.eclipse.org/bugs/show_bug.cgi?id=312502 I also re-created the 2nd patch without the simplification (only the generics update). Thanks, I really want to get VEX going and improving. I would gladly like to be a committer, is there special/particular expectations on that? (In reply to comment #20) > Thanks, I really want to get VEX going and improving. I would gladly like to be > a committer, is there special/particular expectations on that? Fine, the family is growing ;) There are tons of information about "being a committer". A good entry point is here http://www.eclipse.org/legal/committerguidelines.php and here http://www.eclipse.org/legal/newcommitter.php I will start your nomination... (In reply to comment #20) > Thanks, I really want to get VEX going and improving. > I would gladly like to be a committer, > is there special/particular expectations on that? > Yes, you have to spell Vex with lower-case "ex". ;-) I知 happy to see that you are going to become a committer for Vex. One question about your 2nd patch: Have you recreated the model (package: ...provisional.dom.I/impl) or edit it manually? I will review the patch but probably this will take some time... (In reply to comment #22) Ahh, ok. :) The model was recreated, no manual thing done besides the using the EMF ecore editor. (all under emf-src is the by-product, looks like some cosmetical simplifications were done there automatically) (In reply to comment #19) 2nd patch reviewed and applied. Thank you for the contributions. I'm happy that you become a committer. :-)) Closing resolved bugs. Moved to Mylyn Docs Vex. |