Bug 366961

Comment 1 Otavio Pontes 2011-12-17 09:15:41 EST

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.

Comment 2 Andrew Overholt 2011-12-19 08:15:01 EST

(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?

Comment 3 Otavio Pontes 2011-12-19 08:20:31 EST

(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.

Comment 4 Andrew Overholt 2011-12-19 08:28:45 EST

Elliott, Roland, and Jeff:  what do you think of this bug's new summary?

Comment 5 Roland Grunberg 2011-12-19 11:39:35 EST

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.

Comment 6 Jeff Johnston 2011-12-19 12:54:23 EST

(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.

Comment 7 Elliott Baron 2011-12-19 19:03:58 EST

(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.

Comment 8 Roland Grunberg 2012-02-08 15:35:38 EST

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.

Comment 9 Andrew Overholt 2012-04-02 14:27:32 EDT

Should we move ahead with Roland's proposed patch here?

Comment 10 Alexander Kurtakov 2012-04-02 14:59:03 EDT

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.

Comment 11 Roland Grunberg 2012-04-04 17:08:28 EDT

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.