Community
Participate
Working Groups
Build ID: M20071023-1652 Steps To Reproduce: 1. Open an external file in the C editor. 2. Try to set a function breakpoint from the outline view This isn't the typical use case but is the easiest way to see the issue. This happens as well when importing binaries and trying to set breakpoints on the source. The problem is that the TU returned in org.eclipse.cdt.debug.internal.ui.actions.ToggleBreakpointAdapter.getSourceHandle(IDeclaration) is a WorkingCopy rather than an ExternalTranslationUnit. Because there is no IFile for it, it tries to get the path. If this were an ExternalTranslationUnit then it would be fine since ExternalTranslationUnit implements getPath. But since it's really a TranslationUnit, it defaults to the CElement implementation and just returns the project. The easiest fix would be to make WorkingCopy implement getPath just like ExternalTranslationUnit does. The better fix IMO would be to make TranslationUnit implement getPath. In any case, getPath and getLocation do essentially the same thing. That is, return the workspace path if there's an IFile for it, otherwise return the absolute path. The attached patch should address the issue. More information:
Created attachment 89214 [details] proposed patch
(In reply to comment #0) > The better fix IMO would be to make TranslationUnit implement getPath. In any > case, getPath and getLocation do essentially the same thing. That is, return > the workspace path if there's an IFile for it, otherwise return the absolute > path. No they don't do the same thing. Did you look at CElement.getPath()? It never returns the absolute file system location. At best it returns the resource path (ie. the workspace-relative path). The javadoc of the method is not very clear, though. I know this error pretty well, because I once tried exactly the same. Actually, the implementation of getPath() in ExternalTranslationUnit seems to be necessary for the sake of the debugger only. The best solution is to make the debugger use ITranslationUnit.getLocation(), because this returns exactly what the debugger needs.
Created attachment 89231 [details] Fix for ToggleBreakpointAdapter This should solve the breakpoint issue. I'll check all other references to getPath() in the debugger for 5.0
You could fix it by forcing clients to use getLocation, but that probably means changing several places. It also doesn't do much to prevent future bugs. At the very least the java doc for both getPath and getLocation need to be clarified. But I still think making TranslationUnit implement getPath is the better solution. The reason being that if it does have an IFile, it will do exactly what CElement#getPath does. So the behavior should not change at all. But if it doesn't have a resource, it will return what callers are expecting. Also note that the current getLocation implementation calls getPath if there is no IFile. This will never be right as at best it will return the project. And it could cause a stack overflow if the sub class has not set the location.
(In reply to comment #4) > You could fix it by forcing clients to use getLocation, but that probably means > changing several places. It also doesn't do much to prevent future bugs. At > the very least the java doc for both getPath and getLocation need to be > clarified. Javadoc for getLocation() is quite clear. Javadoc for getPath() is confusing to me, too (except for the first sentence). > But I still think making TranslationUnit implement getPath is the better > solution. The reason being that if it does have an IFile, it will do exactly > what CElement#getPath does. So the behavior should not change at all. But if > it doesn't have a resource, it will return what callers are expecting. No, if it has an IFile, it will return IFile.getLocation(), while CElement.getPath() returns IFile.getFullPath(). > Also note that the current getLocation implementation calls getPath if there is > no IFile. This will never be right as at best it will return the project. And > it could cause a stack overflow if the sub class has not set the location. > Right. Calling getPath() in TranslationUnit.getLocation() is wrong. Most probbly it is never called, because for external translation units, the location field is initialized in the constructor.
(In reply to comment #5) > > But I still think making TranslationUnit implement getPath is the better > > solution. The reason being that if it does have an IFile, it will do exactly > > what CElement#getPath does. So the behavior should not change at all. But if > > it doesn't have a resource, it will return what callers are expecting. > No, if it has an IFile, it will return IFile.getLocation(), while > CElement.getPath() returns IFile.getFullPath(). Ah, OK. That's what I was missing. But I still don't think just changing this one call to getLocation from getPath is a good solution. It would fix this particular bug, but how many references are there to getPath? The java doc for ICElement#getPath does imply that it will be a workspace path to an IResource, unless it's in an external archive (??). But it's clearly not being used like that in many cases. If it were, ExternalTranslationUnit would never have implemented getPath. What if we fixed TranslationUnit#getLocation to not call getPath. I think we both agree that this is a bug. Then we implement TranslationUnit#getPath like this: public IPath getPath() { if (getFile() != null) { return super.getPath(); } return getLocation(); } Then of course remove ExternalTranslationUnit#getPath. This way, you get consistent behavior. TranslationUnit's and WorkingCopy's of TranslationUnit's will return exactly what they did before. But now ExternalTranslationUnit's and WorkingCopy's of ExternalTranslationUnit's will now both return the same thing. The only other option I see is to strictly follow the original java doc in ICElement#getPath. But this would require removing ExternalTranslationUnit#getPath and scouring all references to getPath and changing them to getLocation where appropriate. There are 50 references to getPath by my count, excluding any third party plugins using CDT.
(In reply to comment #6) > The only other option I see is to strictly follow the original java doc in > ICElement#getPath. But this would require removing > ExternalTranslationUnit#getPath and scouring all references to getPath and > changing them to getLocation where appropriate. There are 50 references to > getPath by my count, excluding any third party plugins using CDT. I think we're going to have to somehow clean this up regardless, because a lot of these references to getPath may have to change anyway to accomodate the EFS work. I think getPath() should really be getWorkspacePath() (if it's not in the workspace, return null) and getLocation() should be what returns the filesystem path. In the EFS world we probably want to stay away from using either of these and stick to getLocationURI() where possible.
(In reply to comment #7) > (In reply to comment #6) > > The only other option I see is to strictly follow the original java doc in > > ICElement#getPath. But this would require removing > > ExternalTranslationUnit#getPath and scouring all references to getPath and > > changing them to getLocation where appropriate. There are 50 references to > > getPath by my count, excluding any third party plugins using CDT. > > I think we're going to have to somehow clean this up regardless, because a lot > of these references to getPath may have to change anyway to accomodate the EFS > work. > > I think getPath() should really be getWorkspacePath() (if it's not in the > workspace, return null) and getLocation() should be what returns the filesystem > path. In the EFS world we probably want to stay away from using either of > these and stick to getLocationURI() where possible. > I agree that we're going to have to do some major overhaul to properly support EFS. I'll be investigating the EFS stuff on our end here shortly. But that aside for now, I'd like to find a solution for this general problem. I started looking at some of the references to getPath, and there's just too much code right now that assumes getPath will return something useful even when there's no IFile for it.
Created attachment 89276 [details] new proposed patch
(In reply to comment #9) > Created an attachment (id=89276) [details] > new proposed patch I am basically OK with this patch, but I would also encourage to use getLocation() where appropriate and not prolong the dependency on undocumented behaviour of getPath(). There are only two references to getPath() in the debugger. Both assume that getPath() returns an absolute file system location. So why not replace those references with getlocation()?
(In reply to comment #10) > (In reply to comment #9) > > Created an attachment (id=89276) [details] [details] > > new proposed patch > I am basically OK with this patch, but I would also encourage to use > getLocation() where appropriate and not prolong the dependency on undocumented > behaviour of getPath(). > There are only two references to getPath() in the debugger. Both assume that > getPath() returns an absolute file system location. So why not replace those > references with getlocation()? Agreed. Those places should be changed to use getLocation.
Applied patch to cd_4_0 and HEAD. Replaced mentioned occurrences of getPath() with getLocation().
