Bug 256562 - Possibly broken code in TeamContentProviderDescriptor.readExtension(IExtension)
Summary: Possibly broken code in TeamContentProviderDescriptor.readExtension(IExtension)
Status: RESOLVED FIXED
Alias: None
Product: Platform
Classification: Eclipse Project
Component: Team (show other bugs)
Version: 3.5   Edit
Hardware: All All
: P3 normal (vote)
Target Milestone: 3.5 M4   Edit
Assignee: Szymon Brandys CLA
QA Contact:
URL:
Whiteboard:
Keywords: polish
Depends on:
Blocks:
 
Reported: 2008-11-26 04:05 EST by Dani Megert CLA
Modified: 2008-11-28 05:30 EST (History)
2 users (show)

See Also:


Attachments
Fix v01 (3.43 KB, patch)
2008-11-28 04:48 EST, Szymon Brandys CLA
no flags Details | Diff
Fix v02 (5.71 KB, patch)
2008-11-28 05:28 EST, Tomasz Zarna CLA
no flags Details | Diff
mylyn/context/zip (10.04 KB, application/octet-stream)
2008-11-28 05:28 EST, Tomasz Zarna CLA
no flags Details

Note You need to log in before you can comment on or make changes to this bug.
Description Dani Megert CLA 2008-11-26 04:05:08 EST
While doing the dead-code analysis I found this code in
org.eclipse.team.internal.ui.registry.TeamContentProviderDescriptor.readExtension(IExtension):

	for (int i = 0; i < count; i++) {
		IConfigurationElement element = elements[i];
		configElement = element;
		String name = element.getName();
		if (name.equalsIgnoreCase(TAG_TEAM_CONTENT_PROVIDER)) {
			modelProviderId = element.getAttribute(ATT_MODEL_PROVIDER_ID);
			contentExtensionId = element.getAttribute(ATT_CONTENT_EXTENSION_ID);
			String supportsFlatLayoutString = element.getAttribute(ATT_SUPPORTS_FLAT_LAYOUT);
			if (supportsFlatLayoutString != null) {
				supportsFlatLayout = Boolean.valueOf(supportsFlatLayoutString).booleanValue();
			}
			contentProviderName = extension.getLabel();
		}
		break;
	}

Obviously the loop is either useless as it will at most handle elements[0] or it is broken.
Comment 1 Szymon Brandys CLA 2008-11-26 05:24:26 EST
This fancy trick looks like an alternative for the if statement ;-)
Comment 2 Dani Megert CLA 2008-11-26 05:31:46 EST
>This fancy trick looks like an alternative for the if statement ;-)
Does it LOOK LIKE or IS IT?

If it is, then it is a polish bug but otherwise it's simply broken code that needs work and isn't polish.
Comment 3 Szymon Brandys CLA 2008-11-26 06:15:19 EST
It looks like. During M5 I'll take a closer look at it. 
Comment 4 Michael Valenta CLA 2008-11-26 09:10:18 EST
To me, it looks like the code loops through all the elements of the extension until it finds one that is a team content provider. I don't have access to the schema at the moment but it seems to me that the code is valid if the schema allow for multiple unordered elements in the extension, one of which can be a team content provider (or am I missing something?).
Comment 5 Dani Megert CLA 2008-11-26 09:12:52 EST
>To me, it looks like the code loops 
The point is that it DOES NOT loop as the first iteration hits the 'break'
Comment 6 Michael Valenta CLA 2008-11-26 09:15:40 EST
Ah, missed that one (and clearly I missed it when I wrote the code too ;-) The intent was what I described in comment 4.
Comment 7 Dani Megert CLA 2008-11-26 09:18:30 EST
>Ah, missed that one
Yeah, we had such code too in our plug-ins. The Dead code analysis is cool!
Comment 8 Szymon Brandys CLA 2008-11-26 09:21:49 EST
(In reply to comment #6)
> Ah, missed that one (and clearly I missed it when I wrote the code too ;-) The
> intent was what I described in comment 4.
> 

Confusing was that break; was added during API cleaning. This is what the commit comment says at least. Thanks Michael.
Comment 9 Szymon Brandys CLA 2008-11-28 04:48:32 EST
Created attachment 118981 [details]
Fix v01
Comment 10 Tomasz Zarna CLA 2008-11-28 05:28:09 EST
Created attachment 118982 [details]
Fix v02

Slightly modified fix, added a message shown in a situation when the number of elements is different then 1.
Comment 11 Tomasz Zarna CLA 2008-11-28 05:28:18 EST
Created attachment 118983 [details]
mylyn/context/zip
Comment 12 Tomasz Zarna CLA 2008-11-28 05:30:36 EST
Released to HEAD.