Closed
Bug 1442363
Opened 7 years ago
Closed 7 years ago
Constify pointer members of the xpt_struct.h data structures
Categories
(Core :: XPCOM, enhancement)
Core
XPCOM
Tracking
()
RESOLVED
FIXED
mozilla60
Tracking | Status | |
---|---|---|
firefox60 | --- | fixed |
People
(Reporter: mccr8, Assigned: mccr8)
References
Details
Attachments
(8 files)
(deleted),
text/x-review-board-request
|
n.nethercote
:
review+
|
Details |
(deleted),
text/x-review-board-request
|
n.nethercote
:
review+
|
Details |
(deleted),
text/x-review-board-request
|
n.nethercote
:
review+
|
Details |
(deleted),
text/x-review-board-request
|
n.nethercote
:
review+
|
Details |
(deleted),
text/x-review-board-request
|
n.nethercote
:
review+
|
Details |
(deleted),
text/x-review-board-request
|
n.nethercote
:
review+
|
Details |
(deleted),
text/x-review-board-request
|
n.nethercote
:
review+
|
Details |
(deleted),
text/x-review-board-request
|
n.nethercote
:
review+
|
Details |
One step of converting the various data structures in xpt_struct.h into being statically allocated is to mark them const. Fortunately, almost all of the users of these data structures already pretend they are const.
The bulk of the changes are to the deserialization code in xpt_struct.cpp: for the various arrays the code reads in, it has to be written into a local temp array, then that array gets assigned into the const field in the real XPT data structure.
There's no particular reason this can't land early, but it is a little finicky so I won't put my patches up for review until I'm more sure about the viability of bug 1438688.
Comment hidden (mozreview-request) |
Comment hidden (mozreview-request) |
Comment hidden (mozreview-request) |
Comment hidden (mozreview-request) |
Comment hidden (mozreview-request) |
Comment hidden (mozreview-request) |
Comment hidden (mozreview-request) |
Comment hidden (mozreview-request) |
Comment hidden (mozreview-request) |
Comment hidden (mozreview-request) |
Comment hidden (mozreview-request) |
Comment hidden (mozreview-request) |
Comment hidden (mozreview-request) |
Comment hidden (mozreview-request) |
Comment hidden (mozreview-request) |
Comment hidden (mozreview-request) |
Comment hidden (mozreview-request) |
Comment hidden (mozreview-request) |
Comment hidden (mozreview-request) |
Comment hidden (mozreview-request) |
Comment hidden (mozreview-request) |
Comment hidden (mozreview-request) |
Comment hidden (mozreview-request) |
Comment hidden (mozreview-request) |
Assignee | ||
Comment 25•7 years ago
|
||
This has been stable, so I might as well land it. I don't think it is too invasive. This is probably more patches than are really necessary.
Comment 26•7 years ago
|
||
mozreview-review |
Comment on attachment 8955268 [details]
Bug 1442363, part 1 - Constify RegisterXPTHeader().
https://reviewboard.mozilla.org/r/224408/#review231812
Attachment #8955268 -
Flags: review?(n.nethercote) → review+
Comment 27•7 years ago
|
||
mozreview-review |
Comment on attachment 8955269 [details]
Bug 1442363, part 2 - Constify XPTHeader::interface_directory.
https://reviewboard.mozilla.org/r/224410/#review231814
::: xpcom/typelib/xpt/xpt_struct.cpp:149
(Diff revision 3)
> if (!XPT_Do32(cursor, &data_pool))
> return false;
>
> XPT_SetDataOffset(cursor->state, data_pool);
>
> + XPTInterfaceDirectoryEntry* interface_directory = nullptr;
I can't work out why you introduced the local variable, but presumably it's necessary.
Attachment #8955269 -
Flags: review?(n.nethercote) → review+
Comment 28•7 years ago
|
||
mozreview-review |
Comment on attachment 8955270 [details]
Bug 1442363, part 3 - Constify the strings in xpt_struct.h.
https://reviewboard.mozilla.org/r/224412/#review231816
Attachment #8955270 -
Flags: review?(n.nethercote) → review+
Comment 29•7 years ago
|
||
mozreview-review |
Comment on attachment 8955271 [details]
Bug 1442363, part 4 - Constify XPTInterfaceDirectoryEntry::interface_descriptor.
https://reviewboard.mozilla.org/r/224414/#review231818
Attachment #8955271 -
Flags: review?(n.nethercote) → review+
Comment 30•7 years ago
|
||
mozreview-review |
Comment on attachment 8955272 [details]
Bug 1442363, part 5 - Constify XPTInterfaceDescriptor::method_descriptors.
https://reviewboard.mozilla.org/r/224416/#review231820
::: xpcom/reflect/xptinfo/xptiInterfaceInfo.cpp:88
(Diff revision 3)
> mParent = parent;
> if (parent->GetHasNotXPCOMFlag()) {
> SetHasNotXPCOMFlag();
> } else {
> for (uint16_t idx = 0; idx < mDescriptor->num_methods; ++idx) {
> - nsXPTMethodInfo* method = reinterpret_cast<nsXPTMethodInfo*>(
> + const nsXPTMethodInfo* method = reinterpret_cast<const nsXPTMethodInfo*>(
Can this be a static_cast? nsXPTMethodInfo is a subclass of XPTMethodDescriptor.
::: xpcom/reflect/xptinfo/xptiInterfaceInfo.cpp:193
(Diff revision 3)
> *info = nullptr;
> return NS_ERROR_INVALID_ARG;
> }
>
> // else...
> - *info = reinterpret_cast<nsXPTMethodInfo*>
> + *info = reinterpret_cast<const nsXPTMethodInfo*>
ditto
::: xpcom/reflect/xptinfo/xptiInterfaceInfo.cpp:209
(Diff revision 3)
>
> // This is a slow algorithm, but this is not expected to be called much.
> for(uint16_t i = 0; i < mDescriptor->num_methods; ++i)
> {
> const nsXPTMethodInfo* info;
> - info = reinterpret_cast<nsXPTMethodInfo*>
> + info = reinterpret_cast<const nsXPTMethodInfo*>
ditto
Attachment #8955272 -
Flags: review?(n.nethercote) → review+
Comment 31•7 years ago
|
||
mozreview-review |
Comment on attachment 8955273 [details]
Bug 1442363, part 6 - Constify XPTInterfaceDescriptor::const_descriptors.
https://reviewboard.mozilla.org/r/224418/#review231822
Attachment #8955273 -
Flags: review?(n.nethercote) → review+
Comment 32•7 years ago
|
||
mozreview-review |
Comment on attachment 8955274 [details]
Bug 1442363, part 7 - Constify XPTInterfaceDescriptor::additional_types.
https://reviewboard.mozilla.org/r/224420/#review231826
Attachment #8955274 -
Flags: review?(n.nethercote) → review+
Comment 33•7 years ago
|
||
mozreview-review |
Comment on attachment 8955275 [details]
Bug 1442363, part 8 - Constify XPTMethodDescriptor::params.
https://reviewboard.mozilla.org/r/224422/#review231828
Attachment #8955275 -
Flags: review?(n.nethercote) → review+
Assignee | ||
Comment 34•7 years ago
|
||
(In reply to Nicholas Nethercote [:njn] from comment #27)
> I can't work out why you introduced the local variable, but presumably it's
> necessary.
My changes make the array member const, so individual elements of the array member can't be overwritten. But we need to overwrite them to initialize the array. I introduce a local variable that is non-const, so I can write to it. Then, once I'm done initializing it, I can assign that array to the const one in the struct.
(In reply to Nicholas Nethercote [:njn] from comment #30)
> Can this be a static_cast? nsXPTMethodInfo is a subclass of
> XPTMethodDescriptor.
Yeah, I was kind of wondering that myself. I'll fix the cast (unless somehow it doesn't work).
Comment hidden (mozreview-request) |
Comment hidden (mozreview-request) |
Comment hidden (mozreview-request) |
Comment hidden (mozreview-request) |
Comment 39•7 years ago
|
||
Pushed by amccreight@mozilla.com:
https://hg.mozilla.org/integration/autoland/rev/031309abee2c
part 1 - Constify RegisterXPTHeader(). r=njn
https://hg.mozilla.org/integration/autoland/rev/6b7e0ca809e9
part 2 - Constify XPTHeader::interface_directory. r=njn
https://hg.mozilla.org/integration/autoland/rev/ae8424c2ac77
part 3 - Constify the strings in xpt_struct.h. r=njn
https://hg.mozilla.org/integration/autoland/rev/4a4ea07f6467
part 4 - Constify XPTInterfaceDirectoryEntry::interface_descriptor. r=njn
https://hg.mozilla.org/integration/autoland/rev/05a7101669f8
part 5 - Constify XPTInterfaceDescriptor::method_descriptors. r=njn
https://hg.mozilla.org/integration/autoland/rev/9cec783aa764
part 6 - Constify XPTInterfaceDescriptor::const_descriptors. r=njn
https://hg.mozilla.org/integration/autoland/rev/d65876d7d615
part 7 - Constify XPTInterfaceDescriptor::additional_types. r=njn
https://hg.mozilla.org/integration/autoland/rev/e702856c3ec6
part 8 - Constify XPTMethodDescriptor::params. r=njn
Comment 40•7 years ago
|
||
bugherder |
https://hg.mozilla.org/mozilla-central/rev/031309abee2c
https://hg.mozilla.org/mozilla-central/rev/6b7e0ca809e9
https://hg.mozilla.org/mozilla-central/rev/ae8424c2ac77
https://hg.mozilla.org/mozilla-central/rev/4a4ea07f6467
https://hg.mozilla.org/mozilla-central/rev/05a7101669f8
https://hg.mozilla.org/mozilla-central/rev/9cec783aa764
https://hg.mozilla.org/mozilla-central/rev/d65876d7d615
https://hg.mozilla.org/mozilla-central/rev/e702856c3ec6
Status: NEW → RESOLVED
Closed: 7 years ago
status-firefox60:
--- → fixed
Resolution: --- → FIXED
Target Milestone: --- → mozilla60
You need to log in
before you can comment on or make changes to this bug.
Description
•