Bugzilla – Full Text Bug Listing
| Summary: | [Win32] Remove dead code and obsolete comments | ||
|---|---|---|---|
| Product: | [Eclipse Project] Platform | Reporter: | Nikita Nemkin <nikita> |
| Component: | SWT | Assignee: | Nikita Nemkin <nikita> |
| Status: | VERIFIED FIXED | QA Contact: | Niraj Modi <niraj.modi> |
| Severity: | enhancement | ||
| Priority: | P3 | CC: | daniel_megert, lshanmug, niraj.modi, paul-eclipse, sravan.lakkimsetti |
| Version: | 4.15 | ||
| Target Milestone: | 4.15 M3 | ||
| Hardware: | PC | ||
| OS: | Windows All | ||
| See Also: |
https://git.eclipse.org/r/156302 https://git.eclipse.org/c/platform/eclipse.platform.swt.git/commit/?id=3a2c1d63b5030ca10b7e2001ab2210eb232142bf |
||
| Whiteboard: | |||
| Bug Depends on: | |||
| Bug Blocks: | 549414 | ||
|
Description
Nikita Nemkin
New Gerrit change created: https://git.eclipse.org/r/156302 Even if not actively used isn't Device.getLastErrorText() quite handy while developing Windows SWT stuff? (In reply to Paul Pazderski from comment #2) > Even if not actively used isn't Device.getLastErrorText() quite handy while > developing Windows SWT stuff? If you actually need it, I'll put it back. The codes are documented: https://docs.microsoft.com/en-us/windows/win32/debug/system-error-codes and there's also "Microsoft Error Lookup Tool". Not really. I didn't knew yet it exist and last time I had need it I had not thought of search for it. I didn't knew this Error Lookup Tool. Looks handy. Gerrit change https://git.eclipse.org/r/156302 was merged to [master]. Commit: http://git.eclipse.org/c/platform/eclipse.platform.swt.git/commit/?id=3a2c1d63b5030ca10b7e2001ab2210eb232142bf (In reply to Eclipse Genie from comment #5) > Gerrit change https://git.eclipse.org/r/156302 was merged to [master]. > Commit: > http://git.eclipse.org/c/platform/eclipse.platform.swt.git/commit/ > ?id=3a2c1d63b5030ca10b7e2001ab2210eb232142bf Remove the following: * Display.getLastErrorText() - never used. * Canvas.clearArea() - never used. * Control.checkHandle() - pre-XP code to check GWLP_USERDATA. * DirectoryDialog.openCommonFileDialog() - pre-Vista code. * Some "This code is intentionally commented" comments that lost relevance. * All "Use the character encoding for the default locale" comments. * Win98/NT4 code in BidiUtil. Latest Eclipse I-Build is good and Windows JUnits are also good: https://download.eclipse.org/eclipse/downloads/drops4/I20200206-1805/testResults.php (In reply to Eclipse Genie from comment #5) > Gerrit change https://git.eclipse.org/r/156302 was merged to [master]. > Commit: > http://git.eclipse.org/c/platform/eclipse.platform.swt.git/commit/ > ?id=3a2c1d63b5030ca10b7e2001ab2210eb232142bf Above patch removed an internal class BROWSEINFO.java, which as flagged as an API change specifically below part: https://git.eclipse.org/c/platform/eclipse.platform.swt.git/diff/bundles/org.eclipse.swt/Eclipse%20SWT%20PI/win32/org/eclipse/swt/internal/win32/BROWSEINFO.java?id=3a2c1d63b5030ca10b7e2001ab2210eb232142bf It cased below API bundle version error: https://download.eclipse.org/eclipse/downloads/drops4/I20200206-1805/apitools/analysis/html/org.eclipse.swt/report.html Updated Bundle version via below commits https://git.eclipse.org/r/#/c/157469/ https://git.eclipse.org/r/#/c/157471/ Above API tools error is resolved in latest IBuild: https://download.eclipse.org/eclipse/downloads/drops4/I20200211-1800/apitools/analysis/html/org.eclipse.swt/report.html (In reply to Niraj Modi from comment #8) > Above patch removed an internal class BROWSEINFO.java, which as flagged as > an API change specifically below part: > https://git.eclipse.org/c/platform/eclipse.platform.swt.git/diff/bundles/org. > eclipse.swt/Eclipse%20SWT%20PI/win32/org/eclipse/swt/internal/win32/ > BROWSEINFO.java?id=3a2c1d63b5030ca10b7e2001ab2210eb232142bf Shouldn't API tools ignore internal packages? (In reply to Nikita Nemkin from comment #9) > (In reply to Niraj Modi from comment #8) > > Above patch removed an internal class BROWSEINFO.java, which as flagged as > > an API change specifically below part: > > https://git.eclipse.org/c/platform/eclipse.platform.swt.git/diff/bundles/org. > > eclipse.swt/Eclipse%20SWT%20PI/win32/org/eclipse/swt/internal/win32/ > > BROWSEINFO.java?id=3a2c1d63b5030ca10b7e2001ab2210eb232142bf > > Shouldn't API tools ignore internal packages? It does, that's why I marked the comment as obsolete. The reason for the API Tools warning is https://git.eclipse.org/r/#/c/156302/7/bundles/org.eclipse.swt/Eclipse+SWT/win32/org/eclipse/swt/widgets/Shell.java You would have noticed this error (or warning) in your workspace, assuming you have set the correct API baseline (to R4.15). (In reply to Eclipse Genie from comment #5) > Gerrit change https://git.eclipse.org/r/156302 was merged to [master]. Please ignore comment 8 > Commit: > http://git.eclipse.org/c/platform/eclipse.platform.swt.git/commit/ > ?id=3a2c1d63b5030ca10b7e2001ab2210eb232142bf Above commit touched one of the below public API: ------------------------------------------------- List of compatible changes: - The method org.eclipse.swt.widgets.Shell.dispose() has been moved up in the hierarchy https://git.eclipse.org/c/platform/eclipse.platform.swt.git/diff/bundles/org.eclipse.swt/Eclipse%20SWT/win32/org/eclipse/swt/widgets/Shell.java?id=3a2c1d63b5030ca10b7e2001ab2210eb232142bf ------------------------------------------------- API tools flagged it as an error: https://download.eclipse.org/eclipse/downloads/drops4/I20200206-1805/apitools/analysis/html/org.eclipse.swt/report.html Above was resolved via below SWT Bundle Version update: https://git.eclipse.org/c/platform/eclipse.platform.swt.git/commit/?id=1d373db0aac19fff85101e7d4ff7f18f78a3944b API tools error is now resolved: https://download.eclipse.org/eclipse/downloads/drops4/I20200211-1800/apitools/analysis/html/org.eclipse.swt/report.html (In reply to Dani Megert from comment #10) > > The reason for the API Tools warning is > https://git.eclipse.org/r/#/c/156302/7/bundles/org.eclipse.swt/Eclipse+SWT/ > win32/org/eclipse/swt/widgets/Shell.java I'm pretty sure this change is binary compatible. > You would have noticed this error (or warning) in your workspace, assuming > you have set the correct API baseline (to R4.15). Right, but I run with API checker disabled, since I don't change the API and extra warnings don't help. (In reply to Nikita Nemkin from comment #12) > (In reply to Dani Megert from comment #10) > > > > The reason for the API Tools warning is > > https://git.eclipse.org/r/#/c/156302/7/bundles/org.eclipse.swt/Eclipse+SWT/ > > win32/org/eclipse/swt/widgets/Shell.java > I'm pretty sure this change is binary compatible. Sure it is. The API Tools reported that the minor version needs to be increased because there was a change (not a breakage) since the last release. > Right, but I run with API checker disabled, since I don't change the API and > extra warnings don't help. You *did* change the API because Shell#dispose moved up in the hierarchy. You should not disable API Tools. You're right, I should always check with the API builder, however inconvinient it is. Sorry for the trouble. (In reply to Nikita Nemkin from comment #14) > You're right, I should always check with the API builder, however > inconvinient it is. Sorry for the trouble. You could have your main workspace with disabled API Tools and a "test" workspace where you validate your Gerrit changes with enabled API Tools. But I presume this would be more time consuming than just enabling it in the main workspace as suggested to all contributors. Marking Verified. |