Some Eclipse Foundation services are deprecated, or will be soon. Please ensure you've read this important communication.
Bug 323659 - A poblem can happen with ResourceLookupTree adding resource approach
Summary: A poblem can happen with ResourceLookupTree adding resource approach
Status: RESOLVED FIXED
Alias: None
Product: CDT
Classification: Tools
Component: cdt-core (show other bugs)
Version: 7.0   Edit
Hardware: PC Windows XP
: P3 normal (vote)
Target Milestone: 7.0.1   Edit
Assignee: Markus Schorn CLA
QA Contact: Doug Schaefer CLA
URL:
Whiteboard:
Keywords:
Depends on:
Blocks:
 
Reported: 2010-08-25 17:19 EDT by John Liu CLA
Modified: 2010-08-26 11:23 EDT (History)
0 users

See Also:

Attachments
a proposed patch applied to cdt_7_0 branch (1.30 KB, text/plain)
2010-08-25 17:31 EDT, John Liu CLA 		no flags Details
more elegant solution (3.00 KB, patch)
2010-08-26 03:37 EDT, Markus Schorn CLA 		mschorn.eclipse: iplog-
Details | Diff
Show Obsolete (1) View All Add an attachment (proposed patch, testcase, etc.)

Note You need to log in before you can comment on or make changes to this bug.
Description John Liu CLA 2010-08-25 17:19:49 EDT 
Build Identifier: 

ResourceLookupTree uses an inverse file extensions list to do a reverse extension search and decide which file should be added into the tree for future looking up. See Function private void add(IResource res).

The inverse extension list is initialized by taking entire platform content types excluding the cdt registered content types. See function initFileExtensions()

A problem can happen with this approach if someone extends the cdt and needs to define a new content type with C like extension again. Then the C like extensions (i.e .c) will be added into the inversion extension list. As a consequence, any file with C like extensions redefined by the new content type will not added to ResourceLookupTree. This will cause bugs for any function which needs ResourceLookupTree  to look for a resource, for example ErrorParserManager's function findFileName(String)



Reproducible: Always
Comment 1 John Liu CLA 2010-08-25 17:31:45 EDT 
Created attachment 177477 [details]
a proposed patch applied to cdt_7_0 branch

Propose a fix patch for discussion. 

The fix just adds an extension relevant check to cdt default extension list(fDefaultExtensions) for the incoming resource in the function:

    private void add(IResource res)

Actually do we still need to check the inverse extension list fCurrentExtensions? I believe the inverse extension list is much bigger than the default extension list. So why we pick a longer list to search?
Comment 2 Markus Schorn CLA 2010-08-26 03:37:48 EDT 
Created attachment 177499 [details]
more elegant solution

(In reply to comment #1)
> The fix just adds an extension relevant check to cdt default extension
> list(fDefaultExtensions) for the incoming resource ...
A more elegant solution is to make sure that the default-extensions are part
of the cdt-project-extensions, see my patch.

> Actually do we still need to check the inverse extension list
> fCurrentExtensions? I believe the inverse extension list is much bigger than
> the default extension list. So why we pick a longer list to search?
The inverse list is motivated by the fact that in c/c++ you can include files with any extension or files without extension. Therefore in a CDT project the relevant files are the ones that are known c/c++ files (*.c, ...) plus the ones that have an unknown or no extension.

If I read the source code correctly, the problem can occur only when you have a content type that does not derive from one of the cdt-content types and contains *.c as a file pattern. Is that actually the case?
Comment 3 John Liu CLA 2010-08-26 10:40:24 EDT 
(In reply to comment #2)
> Created an attachment (id=177499) [details]
> more elegant solution
> (In reply to comment #1)
> > The fix just adds an extension relevant check to cdt default extension
> > list(fDefaultExtensions) for the incoming resource ...
> A more elegant solution is to make sure that the default-extensions are part
> of the cdt-project-extensions, see my patch.
> > Actually do we still need to check the inverse extension list
> > fCurrentExtensions? I believe the inverse extension list is much bigger than
> > the default extension list. So why we pick a longer list to search?
> The inverse list is motivated by the fact that in c/c++ you can include files
> with any extension or files without extension. Therefore in a CDT project the
> relevant files are the ones that are known c/c++ files (*.c, ...) plus the ones
> that have an unknown or no extension.
> If I read the source code correctly, the problem can occur only when you have a
> content type that does not derive from one of the cdt-content types and
> contains *.c as a file pattern. Is that actually the case?
Thanks, Markus for your quick fix.

Yes. That is the case when another content type also has C like extensions. I agree your fix is more closing to the original logic and it works too. 

When can you check in your patch to cdt_7_0 stream and head?
Comment 4 Markus Schorn CLA 2010-08-26 11:14:54 EDT 
(In reply to comment #3)
> Yes. That is the case when another content type also has C like extensions. 
ok.

> ...
> When can you check in your patch to cdt_7_0 stream and head?

Fixed in 7.0.1 and 8.0 > 20100826.