[Date Prev][Date Next][Thread Prev][Thread Next][Date Index][Thread Index] [List Home]
Re: [p2-dev] Review wanted. Simplifying the IQueryResult/IQuery API

Hi John,
I agree with that and I have actually already made that change.

So now, the former toCollection() (which didn't return a copy) is called unmodifiableSet() and then we have a toSet() that returns a copy. The toArray() is left unchanged.

For the collector, the unmodifiableSet() is an exact copy of the former toCollection().

I think it's better to state that the type is a Set since it has implications on how it can be used. Calls to contains() may be very expensive on other types of collections and the returned set is often used that way.

I didn't include any toList() since it's not really needed.

- thomas


On 2009-12-18 16:26, John Arthorne wrote:

I find it's always better to use the Java Collection API method names where possible. It reduces confusion and makes the API a little easier to use and understand for a newcomer. It also allows for mixing interfaces - creating a class that implements both Collection and IQueryResult, for example. Since the spec of Collection.toArray() clearly states that it returns a copy I don't see a problem with using the name toArray.

     * The returned array will be "safe" in that no references to it are
     * maintained by this collection.  (In other words, this method must
     * allocate a new array even if this collection is backed by an array).
     * The caller is thus free to modify the returned array.<p>

John



Thomas Hallgren <thomas@xxxxxxx>
Sent by: p2-dev-bounces@xxxxxxxxxxx

12/17/2009 05:36 PM

Please respond to
P2 developer discussions <p2-dev@xxxxxxxxxxx>

To
p2-dev@xxxxxxxxxxx
cc

Subject
Re: [p2-dev] Review wanted. Simplifying the IQueryResult/IQuery API







Ian, Pascal,
I'll add the two methods, and also rename the 'toArray()' method to 'copyAsArray()' for consistency. This means that some of the current use of 'iterator()' can be optimized and I will look into changing that also.

Does that sound OK?

Regards,
Thomas Hallgren



On 2009-12-17 19:35, Pascal Rapicault wrote:

I have reviewed the changes and agree that isEmpty is most the time good enough, and the use of iterator instead of the former toCollection does the job most of the time (looking at the code I think toCollection must have been introduced before we got iterator() which is why we had all this copying).
I'm not quite sure if these 2 new methods would completely fulfil the current needs but it worth a look

Inactive hide details for Thomas Hallgren ---17/12/2009 07:07:06 PM---Thinking more about this.Thomas Hallgren ---17/12/2009 07:07:06 PM---Thinking more about this.


From:

Thomas Hallgren
<thomas@xxxxxxx>

To:

p2-dev@xxxxxxxxxxx

Date:

17/12/2009 07:07 PM

Subject:

Re: [p2-dev] Review wanted. Simplifying the IQueryResult/IQuery API





Thinking more about this.

What I want to avoid is that the developer gets the wrong impression
when he looks at the IQueryResult. If it has a size() method, it's
somewhat implicit that it is lightweight. I've seen one size() too many
in the test of a loop construct for instance :-)

What I think is really needed, is what you mention, the ability to
create a collection in a way that, if possible, avoids reallocation as
the collection grows. So how about this?

Instead of the toCollection() method (the name says very little on
what's going on), we add these two methods:

/**
* Creates a new List copy with the contents of this IQueryResult. The
* copy can be altered without any side effects on the IQueryResult.
*/
List copyAsList();

/**
* Creates a new Set copy with the contents of this IQueryResult. The
* copy can be altered without any side effects on the IQueryResult.
*/
Set copyAsSet();

This would make the user understand that what he gets is indeed a copy.
And since it's either a Set or a List, he can also use that copy
directly for various purposes. That would remove the need for a size()
method.

What do you think about that?

- thomas




On 2009-12-17 18:30, Thomas Hallgren wrote:
> On 2009-12-17 18:26, Pascal Rapicault wrote:
>>
>> I have not reviewed the patch but I'm not a big fan of the removal of
>> size and toCollection.
>> They end up being particularly useful in some places, especially to
>> convert from one type to the other, or eventually create collections
>> with the right size up front (which saves a lot of growth when you
>> work on very large collections).
>>
>> I agree that there are cases where all the IUs can't be held in
>> memory, however there are cases in today's code (engine and planner)
>> where I don't see how not having all the IUs in memory is possible,
>> and where the presence of such methods is necessary.
>> In the end I think I could probably leave without the toCollection,
>> but loosing size would likely cause performance issues.
>>
> I doubt that. I think it's rather the opposite given the number of
> unnecessary collection copies that were removed.
>
> Is there any way we can compare performance, with and without my
> patch? Any particular scenarios that you are worried about?
>
> - thomas
>
> _______________________________________________
> p2-dev mailing list
>
p2-dev@xxxxxxxxxxx
>
https://dev.eclipse.org/mailman/listinfo/p2-dev

_______________________________________________
p2-dev mailing list

p2-dev@xxxxxxxxxxx
https://dev.eclipse.org/mailman/listinfo/p2-dev




_______________________________________________
p2-dev mailing list
p2-dev@xxxxxxxxxxx
https://dev.eclipse.org/mailman/listinfo/p2-dev
 

_______________________________________________
p2-dev mailing list
p2-dev@xxxxxxxxxxx
https://dev.eclipse.org/mailman/listinfo/p2-dev

_______________________________________________ p2-dev mailing list p2-dev@xxxxxxxxxxx https://dev.eclipse.org/mailman/listinfo/p2-dev

GIF image

GIF image

GIF image

GIF image

GIF image

GIF image

GIF image

GIF image

GIF image