Closed
Bug 1163916
Opened 9 years ago
Closed 9 years ago
Remove two static constructors from XPCOM
Categories
(Core :: XPCOM, defect)
Core
XPCOM
Tracking
()
RESOLVED
FIXED
mozilla41
Tracking | Status | |
---|---|---|
firefox41 | --- | fixed |
People
(Reporter: n.nethercote, Assigned: n.nethercote)
Details
Attachments
(2 files)
1.21 KB,
patch
|
froydnj
:
review+
|
Details | Diff | Splinter Review |
3.90 KB,
patch
|
froydnj
:
review+
|
Details | Diff | Splinter Review |
Some low-hanging fruit, here.
Assignee | ||
Comment 1•9 years ago
|
||
Attachment #8604505 -
Flags: review?(nfroyd)
Comment 2•9 years ago
|
||
Comment on attachment 8604505 [details] [diff] [review] (part 1) - Avoid a static nsDebugImpl constructor Review of attachment 8604505 [details] [diff] [review]: ----------------------------------------------------------------- I think in this case it might be easier (and less confusing, given that somebody will wonder why |sImpl| isn't a strong reference) to simply default nsDebugImpl's constructor: class nsDebugImpl ... { nsDebugImpl() = default; ... } The compiler understands |= default|, whereas even an empty constructor causes static constructors. (This is true for GCC; I think Clang and MSVC are smart enough to handle both identically.) r=me with that change. ::: xpcom/base/nsDebugImpl.cpp @@ -578,5 @@ > #warning do not know how to break on this platform > #endif > } > > -static const nsDebugImpl kImpl; Ah, yes, this is the infamous "static xpcom instance with special AddRef/Release functions".
Attachment #8604505 -
Flags: review?(nfroyd) → review+
Assignee | ||
Comment 3•9 years ago
|
||
Oh, I forgot to upload this one yesterday.
Attachment #8604938 -
Flags: review?(nfroyd)
Comment 4•9 years ago
|
||
Comment on attachment 8604938 [details] [diff] [review] (part 2) - Avoid a static constructor for kCompressedBufferLength Review of attachment 8604938 [details] [diff] [review]: ----------------------------------------------------------------- r=me, your call on whether to change the below. ::: xpcom/io/SnappyUncompressInputStream.cpp @@ +23,5 @@ > + if (kCompressedBufferLength == 0) { > + kCompressedBufferLength = > + detail::SnappyFrameUtils::MaxCompressedBufferLength(snappy::kBlockSize); > + MOZ_ASSERT(kCompressedBufferLength > 0); > + } You can just do: static size_t kCompressedBufferLength = detail::...; MOZ_ASSERT(kCompressedBufferLength > 0); return kCompressedBufferLength; I suppose that needlessly MOZ_ASSERTs, but I don't suspect this code is particularly performance sensitive in tests. And this way should be ever-so-slightly more efficient in release anyway...
Attachment #8604938 -
Flags: review?(nfroyd) → review+
Assignee | ||
Comment 5•9 years ago
|
||
> You can just do:
>
> static size_t kCompressedBufferLength = detail::...;
>
> MOZ_ASSERT(kCompressedBufferLength > 0);
> return kCompressedBufferLength;
So if the |static size_t kCompressedBufferLength = ...| is at the top-level it causes a static ctor, but if it's within a function it doesn't? I wouldn't have believed it if count_ctors.py didn't confirm it. How interesting.
Assignee | ||
Comment 6•9 years ago
|
||
glandium explained to me how this works -- |static| variables in a function are initialized upon the first call, not at start-up. That means I'll be able to avoid some tests in bug 1164373 and maybe some of my other patches. Nice.
https://hg.mozilla.org/integration/mozilla-inbound/rev/eee8a7f02b1a https://hg.mozilla.org/integration/mozilla-inbound/rev/4d742ed5c563
https://hg.mozilla.org/mozilla-central/rev/eee8a7f02b1a https://hg.mozilla.org/mozilla-central/rev/4d742ed5c563
Status: ASSIGNED → RESOLVED
Closed: 9 years ago
Resolution: --- → FIXED
Target Milestone: --- → mozilla41
You need to log in
before you can comment on or make changes to this bug.
Description
•