Closed Bug 1127321 Opened 7 years ago Closed 7 years ago

TileClient copy constructor and operator= missing the expiration tracker member

Categories

(Core :: Graphics: Layers, defect)

x86_64
Linux
defect
Not set
normal

Tracking

()

RESOLVED INVALID

People

(Reporter: nical, Assigned: nical)

Details

Attachments

(1 file)

I assume this is unintentional, and bad.
Attachment #8556476 - Flags: review?(jmuizelaar)
Should TileClient just be using the default operators here?
(In reply to Jeff Muizelaar [:jrmuizel] from comment #2)
> Should TileClient just be using the default operators here?

Nope the PrivateProtector prevents that.
Comment on attachment 8556476 [details] [diff] [review]
add missing member copy

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

How did you find this?
Attachment #8556476 - Flags: review?(jmuizelaar) → review+
(In reply to Jeff Muizelaar [:jrmuizel] from comment #4)
> Comment on attachment 8556476 [details] [diff] [review]
> add missing member copy
> 
> Review of attachment 8556476 [details] [diff] [review]:
> -----------------------------------------------------------------
> 
> How did you find this?

I made the same mistake implementing edge padding for DrawTargetTiled (I had to add a member and forgot about the copy constructor). Then I decided that from now on I dislike explicit copy constructors.
Is there a warning that we could turn on that would check whether people assign all members in copy constructors and assignment operators?
(In reply to Nicolas Silva [:nical] from comment #5)
> (In reply to Jeff Muizelaar [:jrmuizel] from comment #4)
> > Comment on attachment 8556476 [details] [diff] [review]
> > add missing member copy
> > 
> > Review of attachment 8556476 [details] [diff] [review]:
> > -----------------------------------------------------------------
> > 
> > How did you find this?
> 
> I made the same mistake implementing edge padding for DrawTargetTiled (I had
> to add a member and forgot about the copy constructor). Then I decided that
> from now on I dislike explicit copy constructors.
> Is there a warning that we could turn on that would check whether people
> assign all members in copy constructors and assignment operators?

I've started work on a clang analysis pass that requires you to initialize all members in constructors
unless that member has a default constructor or is annotated as not requiring initialization. It should be possible to extend this so that it checks the copy constructors, move constructors, assignment operators etc.
(In reply to Jeff Muizelaar [:jrmuizel] from comment #6)
> I've started work on a clang analysis pass that requires you to initialize
> all members in constructors
> unless that member has a default constructor or is annotated as not
> requiring initialization. It should be possible to extend this so that it
> checks the copy constructors, move constructors, assignment operators etc.

Tremendous.
Comment on attachment 8556476 [details] [diff] [review]
add missing member copy

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

I think there might be no bug here.
Attachment #8556476 - Flags: review+ → review-
I believe nsExpirationTracker already properly sets this.
(In reply to Jeff Muizelaar [:jrmuizel] from comment #10)
> I think there might be no bug here.

Hm, so if I understand correctly, this works because we know that by the end of this spin of the event-loop (assuming the expiration notifications come in a separate event-loop turn), there will be only one of the two copies of the tile that just got copied/assigned, so there is no risk of the same tile expiring twice (through 2 copies of the POD struct) which would lead to TextureClients being added twice to the pool.
Status: NEW → RESOLVED
Closed: 7 years ago
Resolution: --- → INVALID
You need to log in before you can comment on or make changes to this bug.