Hi Sakari,
On 06/10/2013 12:15 AM, Sakari Ailus wrote:
> Sylwester Nawrocki wrote:
> ...
>>>> diff --git a/drivers/media/media-entity.c b/drivers/media/media-entity.c
>>>> index e1cd132..bd85dc3 100644
>>>> --- a/drivers/media/media-entity.c
>>>> +++ b/drivers/media/media-entity.c
>>>> @@ -429,6 +429,57 @@ media_entity_create_link(struct media_entity
>>>> *source, u16 source_pad,
>>>> }
>>>> EXPORT_SYMBOL_GPL(media_entity_create_link);
>>>>
>>>> +void __media_entity_remove_links(struct media_entity *entity)
>>>> +{
>>>> + int i, r;
>>>
>>> u16? r can be defined inside the loop.
>>
>> I would argue 'unsigned int' would be more optimal type for i in most
>> cases. Will move r inside the loop.
>>
>>>> + for (i = 0; i< entity->num_links; i++) {
>>>> + struct media_link *link =&entity->links[i];
>>>> + struct media_entity *remote;
>>>> + int num_links;
>>>
>>> num_links is u16 in struct media_entity. I'd use the same type.
>>
>> I would go with 'unsigned int', as a more natural type for the CPU in
>> most cases. It's a minor issue, but I can't see how u16 would have been
>> more optimal than unsigned int for a local variable like this, while
>> this code is mostly used on 32-bit systems at least.
>>
>>>> + if (link->source->entity == entity)
>>>> + remote = link->sink->entity;
>>>> + else
>>>> + remote = link->source->entity;
>>>> +
>>>> + num_links = remote->num_links;
>>>> +
>>>> + for (r = 0; r< num_links; r++) {
>>>
>>> Is caching remote->num_links needed, or do I miss something?
>>
>> Yes, it is needed, remote->num_links is decremented inside the loop.
>
> Oh, forgot this one... yes, remote->num_links is decremented, but so is
> r it's compared to. So I don't think it's necessary to cache it, but
> it's neither an error to do so.
Indeed, it seems more correct to not cache it, thanks.
>>>> + struct media_link *rlink =&remote->links[r];
>>>> +
>>>> + if (rlink != link->reverse)
>>>> + continue;
>>>> +
>>>> + if (link->source->entity == entity)
>>>> + remote->num_backlinks--;
>>>> +
>>>> + remote->num_links--;
>>>> +
>>>> + if (remote->num_links< 1)
>>>
>>> How about: if (!remote->num_links) ?
>>
>> Hmm, perhaps squashing the above two lines to:
>>
>> if (--remote->num_links == 0)
>>
>> ?
>
> I'm not too fond of --/++ operators as part of more complex structures
> so I'd keep it separate. Fewer lines doesn't always equate to more
> readable code. :-)
In general I agree, however it's quite simple statement in this case -
only decrement and test, it's often only one instruction even in the
assembly language...
I'm going to squash following to this patch:
diff --git a/drivers/media/media-entity.c b/drivers/media/media-entity.c
index f5ea82e..1fb619d 100644
--- a/drivers/media/media-entity.c
+++ b/drivers/media/media-entity.c
@@ -436,18 +436,18 @@ void __media_entity_remove_links(struct media_entity *entity)
for (i = 0; i < entity->num_links; i++) {
struct media_link *link = &entity->links[i];
struct media_entity *remote;
- unsigned int r, num_links;
+ unsigned int r;
if (link->source->entity == entity)
remote = link->sink->entity;
else
remote = link->source->entity;
- num_links = remote->num_links;
-
- for (r = 0; r < num_links; r++) {
- if (remote->links[r] != link->reverse)
+ while (r < remote->num_links; r++) {
+ if (remote->links[r] != link->reverse) {
+ r++;
continue;
+ }
if (link->source->entity == entity)
remote->num_backlinks--;
@@ -456,7 +456,7 @@ void __media_entity_remove_links(struct media_entity *entity)
break;
/* Insert last entry in place of the dropped link. */
- remote->links[r--] = remote->links[remote->num_links];
+ remote->links[r] = remote->links[remote->num_links];
}
}
So the loop looks something like:
while (r < remote->num_links) {
if (remote->links[r] != link->reverse) {
r++;
continue;
}
if (link->source->entity == entity)
remote->num_backlinks--;
if (--remote->num_links == 0)
break;
/* Insert last entry in place of the dropped link. */
remote->links[r] = remote->links[remote->num_links];
}
Does it sound OK ?
Regards,
Sylwester
--
To unsubscribe from this list: send the line "unsubscribe linux-media" in
the body of a message to majordomo@xxxxxxxxxxxxxxx
More majordomo info at http://vger.kernel.org/majordomo-info.html
