Community
Participate
Working Groups
Resource: org.eclipse.linuxtools.valgrind.launch/src/org/eclipse/linuxtools/internal/valgrind/launch/ValgrindLaunchPlugin.java Location: line 141 It looks like commit 2ce832d53f111a53fc2b23d903c4b700c684fd9c introduced a return type that isn't API for a public (well, it went from protected to public) method. Since we're trying to stabilize API for 1.0, could we get around this by marking the tests plugin (what I understood was the reason for this change) as x-friends in MANIFEST.MF?
I couldn't access a protected method even when I added the plugin in the x-friend list. But I figured out a way to do the tests without calling the method, so I change the visibility back to protected. (commit: c0bc29832e91814ee5413b704bef9dd666d0221c) When I was fixing this bug I realized that the internal package org.eclipse.linuxtools.internal.valgrind.launch was exported in Export-Package directive. Shouldn't we add a x-friends list with the packages we know that uses it? So people who tries to access it outside of linuxtools will be warned.
(In reply to comment #1) > I couldn't access a protected method even when I added the plugin in the > x-friend list. But I figured out a way to do the tests without calling the > method, so I change the visibility back to protected. (commit: > c0bc29832e91814ee5413b704bef9dd666d0221c) Great, thanks! > When I was fixing this bug I realized that the internal package > org.eclipse.linuxtools.internal.valgrind.launch was exported in Export-Package > directive. Shouldn't we add a x-friends list with the packages we know that > uses it? So people who tries to access it outside of linuxtools will be warned. Should it be exported at all? If it should be, then you're right that we should have an x-friends list. That or make it official API. What do you think is best?
(In reply to comment #2) > (In reply to comment #1) > > I couldn't access a protected method even when I added the plugin in the > > x-friend list. But I figured out a way to do the tests without calling the > > method, so I change the visibility back to protected. (commit: > > c0bc29832e91814ee5413b704bef9dd666d0221c) > > Great, thanks! > > > When I was fixing this bug I realized that the internal package > > org.eclipse.linuxtools.internal.valgrind.launch was exported in Export-Package > > directive. Shouldn't we add a x-friends list with the packages we know that > > uses it? So people who tries to access it outside of linuxtools will be warned. > > Should it be exported at all? If it should be, then you're right that we > should have an x-friends list. That or make it official API. What do you > think is best? I'm not sure. I just found it weird because the package is called internal. It is used by several plugins in valgrind package, but I have no idea if anyone is using it outside valgrind.
Elliott, Roland, and Jeff: what do you think of this bug's new summary?
The x-friends lists for o.e.l.internal.valgrind.{launch,core} would be fairly large, so it seems like exposing it as public might be the way to go.
(In reply to comment #4) > Elliott, Roland, and Jeff: what do you think of this bug's new summary? I don't think everything in internal should be exposed. You want to err on the side of caution. In general, I usually expose the plugin so there is some way to expose internal stuff if needed, but that is just my practice. In this particular case, there is no way to access anything via public APIs other than interfaces. The internal package should exist and be hidden from all but friends which at least appears to be a number of Valgrind test packages. If certain interfaces should be moved from internal to public, that is certainly an option.
(In reply to comment #6) > (In reply to comment #4) > > Elliott, Roland, and Jeff: what do you think of this bug's new summary? > > I don't think everything in internal should be exposed. You want to err on the > side of caution. In general, I usually expose the plugin so there is some way > to expose internal stuff if needed, but that is just my practice. In this > particular case, there is no way to access anything via public APIs other than > interfaces. > > The internal package should exist and be hidden from all but friends which at > least appears to be a number of Valgrind test packages. If certain interfaces > should be moved from internal to public, that is certainly an option. I'm with Jeff on this. There is a lot of code in valgrind.launch, exposing all of that as public API would certainly cause problems later down the road. From my point of view, the Valgrind tests plugin should be marked x-friends. This plugin is meant to be used to create tests for each of the Valgrind tools. Anything that is tool-specific: memcheck, massif, cachegrind, helgrind plugins, I'd like to keep as not x-friends. The reasoning is that anyone should be able to create a new tool plugin and associated tests using only public API.
Created attachment 210755 [details] Patch I've played with the x-friends options and found the proposed patch solves the API problems. I found that I had to have the x-friend relation going both ways to fix the API issues. ie. If a class (in this case internal) was using another internal class from another bundle, both had to specify in their bundle that the other was an x-friend.
Should we move ahead with Roland's proposed patch here?
Tests are fragments now so they have access to everything that the plugin has access to. The x-friend for tests shouldn't be needed anymore.
Commits 87673ccaf69c11832269d1129e4a8a599c2f39b7 (mentioned by Alex) and ae81f337560d61f30272ae4ce04f88a96138d423 contribute towards ensuring that no warnings/errors are issued when internal valgrind classes are accessed by other valgrind plug-ins. Resolving as fixed.
