use UniquePtr for managing pointers where possible.

RESOLVED FIXED in Firefox 53

Status

()

Core
Gecko Profiler
RESOLVED FIXED
a year ago
a year ago

People

(Reporter: froydnj, Assigned: froydnj)

Tracking

unspecified
mozilla53
Points:
---
Dependency tree / graph

Firefox Tracking Flags

(firefox53 fixed)

Details

Attachments

(3 attachments)

I probably won't catch everything, but I have a patch series to fix a goodly number of places.
Depends on: 1142197
Created attachment 8823387 [details] [diff] [review]
make ThreadInfo::mName a UniqueFreePtr

Smart pointers are better than raw pointers.
Attachment #8823387 - Flags: review?(mstange)
Created attachment 8823388 [details] [diff] [review]
make ThreadInfo manage mProfile with UniquePtr

Smart pointers are better than raw pointers.

I think I like this patch, but there is also some weirdness that ThreadInfo owns
ThreadProfile objects, and ThreadProfile objects can own ThreadInfo objects
(though apparently not at the same time?).  We should sort that out.
Attachment #8823388 - Flags: review?(mstange)
Created attachment 8823389 [details] [diff] [review]
manage Sampler::PlatformData with UniquePtr

Smart pointers are better than raw pointers, and this makes clients of
PlatformData slightly simpler because they don't have to manage
destruction themselves: the new UniquePtr-derived type handles all of
that for us.
Attachment #8823389 - Flags: review?(mstange)
Attachment #8823387 - Flags: review?(mstange) → review+
Comment on attachment 8823388 [details] [diff] [review]
make ThreadInfo manage mProfile with UniquePtr

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

I don't know about the the ThreadInfo vs ThreadProfile ownership situation, but this patch seems like it should be safe either way.
Attachment #8823388 - Flags: review?(mstange) → review+
Comment on attachment 8823389 [details] [diff] [review]
manage Sampler::PlatformData with UniquePtr

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

Looks good.
It still feels somewhat unnecessary complex - we're abstracting something out that's known at compile time, right?
It may also be unused, because I don't see any callers of GetProfiledThread.
Attachment #8823389 - Flags: review?(mstange) → review+
Blocks: 1328363

Comment 6

a year ago
Pushed by nfroyd@mozilla.com:
https://hg.mozilla.org/integration/mozilla-inbound/rev/41e0e99837f9
part 1 - make ThreadInfo::mName a UniqueFreePtr; r=mstange
https://hg.mozilla.org/integration/mozilla-inbound/rev/07f764d10078
part 2 - make ThreadInfo manage mProfile with UniquePtr; r=mstange
https://hg.mozilla.org/integration/mozilla-inbound/rev/e1a7c28606f1
part 3 - manage Sampler::PlatformData with UniquePtr; r=mstange

Comment 7

a year ago
bugherder
https://hg.mozilla.org/mozilla-central/rev/41e0e99837f9
https://hg.mozilla.org/mozilla-central/rev/07f764d10078
https://hg.mozilla.org/mozilla-central/rev/e1a7c28606f1
Status: NEW → RESOLVED
Last Resolved: a year ago
status-firefox53: --- → fixed
Resolution: --- → FIXED
Target Milestone: --- → mozilla53
You need to log in before you can comment on or make changes to this bug.