Closed
Bug 1223808
Opened 6 years ago
Closed 5 years ago
replace nsAutoArrayPtr<T> with UniquePtr<T[]> in netwerk/
Categories
(Core :: Networking, defect)
Core
Networking
Tracking
()
RESOLVED
FIXED
mozilla45
Tracking | Status | |
---|---|---|
firefox45 | --- | fixed |
People
(Reporter: froydnj, Assigned: froydnj)
References
Details
Attachments
(3 files, 1 obsolete file)
2.06 KB,
patch
|
michal
:
review+
|
Details | Diff | Splinter Review |
57.38 KB,
patch
|
mcmanus
:
review+
|
Details | Diff | Splinter Review |
3.54 KB,
patch
|
michal
:
review+
|
Details | Diff | Splinter Review |
No description provided.
![]() |
Assignee | |
Comment 1•6 years ago
|
||
This function: - Allocates a block of memory; - Copies things into that block of memory; - Allocates another block of memory inside nsAutoCString; - Copies into our string from the block of memory we just allocated. Clearly we can avoid a memory allocation and some copying by just allocating the amount of data we need in the string first, and copying things into the string.
Attachment #8686061 -
Flags: review?(mcmanus)
![]() |
Assignee | |
Comment 2•6 years ago
|
||
Converting HandleHashKey's use of nsAutoArrayPtr is a little difficult, because: MakeUnique<SHA1Sum::Hash>(); doesn't work, and trying to do something like: mHash.reset(reinterpret_cast<SHA1Sum::Hash*>(new uint8_t[SHA1Sum::kHashSize])); is just ugly. Since we're always allocating it anyway, let's just store it directly in HandleHashKey and avoid the dynamic allocation in the first place.
Attachment #8686062 -
Flags: review?(michal.novotny)
![]() |
Assignee | |
Comment 3•6 years ago
|
||
Attachment #8686063 -
Flags: review?(mcmanus)
Updated•6 years ago
|
Attachment #8686061 -
Flags: review?(mcmanus) → review+
Comment 4•6 years ago
|
||
Comment on attachment 8686062 [details] [diff] [review] part 2 - don't dynamically allocate HandleHashKey::mHash Review of attachment 8686062 [details] [diff] [review]: ----------------------------------------------------------------- The reason why the hash is dynamically allocated is that in CacheFileHandle we need a pointer to the hash that won't change. If the hash is stored in HandleHashKey, then CacheFileHandle::mHash becomes invalid sooner or later, right?
Attachment #8686062 -
Flags: review?(michal.novotny) → review-
![]() |
Assignee | |
Comment 5•6 years ago
|
||
(In reply to Michal Novotny (:michal) from comment #4) > The reason why the hash is dynamically allocated is that in CacheFileHandle > we need a pointer to the hash that won't change. If the hash is stored in > HandleHashKey, then CacheFileHandle::mHash becomes invalid sooner or later, > right? Ah, yes. This explains the crashes I was getting shortly after startup, sometime after I posted this patch. :) I guess the alternative is some liberal use of reinterpret_cast.
Comment 6•6 years ago
|
||
Comment on attachment 8686063 [details] [diff] [review] part 3 - replace nsAutoArrayPtr<T> with UniquePtr<T[]> in netwerk/ Review of attachment 8686063 [details] [diff] [review]: ----------------------------------------------------------------- thanks.. that will get picked up as an idiom
Attachment #8686063 -
Flags: review?(mcmanus) → review+
![]() |
Assignee | |
Comment 7•6 years ago
|
||
Here's a patch that seems to work better.
Attachment #8689187 -
Flags: review?(michal.novotny)
![]() |
Assignee | |
Updated•6 years ago
|
Attachment #8686062 -
Attachment is obsolete: true
Updated•5 years ago
|
Attachment #8689187 -
Flags: review?(michal.novotny) → review+
https://hg.mozilla.org/integration/mozilla-inbound/rev/689d9f449560 https://hg.mozilla.org/integration/mozilla-inbound/rev/37997f6e4349 https://hg.mozilla.org/integration/mozilla-inbound/rev/cc847f227d9a
Comment 9•5 years ago
|
||
bugherder |
https://hg.mozilla.org/mozilla-central/rev/689d9f449560 https://hg.mozilla.org/mozilla-central/rev/37997f6e4349 https://hg.mozilla.org/mozilla-central/rev/cc847f227d9a
Status: NEW → RESOLVED
Closed: 5 years ago
status-firefox45:
--- → fixed
Resolution: --- → FIXED
Target Milestone: --- → mozilla45
You need to log in
before you can comment on or make changes to this bug.
Description
•