Closed Bug 1421826 Opened 2 years ago Closed 1 year ago

ContainerState::ProcessDisplayItems: Unused value (UNUSED_VALUE)

Categories

(Core :: Web Painting, enhancement, P3, trivial)

enhancement

Tracking

()

RESOLVED INVALID

People

(Reporter: Sylvestre, Unassigned, Mentored)

References

(Blocks 1 open bug)

Details

(Keywords: coverity, good-first-bug, Whiteboard: [good first bug][lang=C++][CID 1419983])

Attachments

(1 file)

A trivial good first bug to understand the workflows.
returned_pointer: Assigning value from iter.GetNext() to i here, but that stored value is overwritten before it can be used.


https://dxr.mozilla.org/mozilla-central/source/layout/painting/FrameLayerBuilder.cpp?q=FrameLayerBuilder.cpp&redirect_type=direct#4018
Mentor: sledru
Hi, I would like to work on this bug.
sure, please submit a patch!
Thank you, Samuel! :)
Assignee: nobody → samuel.chen
Comment on attachment 8933618 [details] [diff] [review]
not storing the value of iter.GetNext()

I am not certain about the second change but it looks good.
You should now find for an official reviewer!
Attachment #8933618 - Flags: feedback+
Comment on attachment 8933618 [details] [diff] [review]
not storing the value of iter.GetNext()

Review of attachment 8933618 [details] [diff] [review]:
-----------------------------------------------------------------

I'm actually not a huge fan of this warning, since if anyone tries to use 'i' (even though nobody is now), leaving it as the old value will be incorrect. I guess that's my fault for not having a proper iterator instance.
Attachment #8933618 - Flags: review+
Priority: -- → P3
Hi all,

As Matt mentioned above, currently there is no one use this variable "i", could I simply just replace the declaration in while loop condition: "nsDisplayItem* i" to "nsDisplayItem* item" on line 3989 and remove line 3990 like

> @@ -3986,8 +3986,7 @@
>    int layerCount = 0;
>  
>    FlattenedDisplayItemIterator iter(mBuilder, aList);
> -  while (nsDisplayItem* i = iter.GetNext()) {
> -    nsDisplayItem* item = i;
> +  while (nsDisplayItem* item = iter.GetNext()) {
>      MOZ_ASSERT(item);
> 
>     DisplayItemType itemType = item->GetType();

I'm not sure if it will affect any flexibility for this function/module, how do you think?
ni for comment 7.
Flags: needinfo?(matt.woodrow)
(In reply to samuel from comment #7)
> Hi all,
> 
> As Matt mentioned above, currently there is no one use this variable "i",
> could I simply just replace the declaration in while loop condition:
> "nsDisplayItem* i" to "nsDisplayItem* item" on line 3989 and remove line
> 3990 like
> 
> > @@ -3986,8 +3986,7 @@
> >    int layerCount = 0;
> >  
> >    FlattenedDisplayItemIterator iter(mBuilder, aList);
> > -  while (nsDisplayItem* i = iter.GetNext()) {
> > -    nsDisplayItem* item = i;
> > +  while (nsDisplayItem* item = iter.GetNext()) {
> >      MOZ_ASSERT(item);
> > 
> >     DisplayItemType itemType = item->GetType();
> 
> I'm not sure if it will affect any flexibility for this function/module, how
> do you think?

Yes, that sounds reasonable.
Flags: needinfo?(matt.woodrow)

There's a r+ patch which didn't land and no activity in this bug for 2 weeks.
:samuel.chen, could you have a look please?

Flags: needinfo?(samuel.chen)

Unassigning
Samuel, if you are are still interested, please try to address the issue

Assignee: samuel.chen → nobody
Severity: normal → trivial

Hey i'm new here, is it okay if i pick this up?

Sure, as nothing happened for the last 11 months, it is safe

Ahh, I'll get to it then :)

Hey it seems that the link is outdated. I can't seem to locate the code referenced here. Can you please help me out? :)

@sylvestre, is this still an outstanding issue? Looking at the commit history on the file, it appears that all the references to the iterator "i" variable have been eliminated between several diffs submitted over the past few months. Sample Diff, Current File.

Just wanted to follow up on this. Based on my findings in Comment 16, is this still an open issue?

No, this code has all been rewritten since the bug was filed sorry.

Status: NEW → RESOLVED
Closed: 1 year ago
Resolution: --- → INVALID
Flags: needinfo?(samuel.chen)
You need to log in before you can comment on or make changes to this bug.