Closed Bug 1442363 Opened 2 years ago Closed 2 years ago

Constify pointer members of the xpt_struct.h data structures

Categories

(Core :: XPCOM, enhancement)

enhancement
Not set

Tracking

()

RESOLVED FIXED
mozilla60
Tracking Status
firefox60 --- fixed

People

(Reporter: mccr8, Assigned: mccr8)

References

Details

Attachments

(8 files)

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.
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 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 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 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 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 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 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 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 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+
(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).
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
You need to log in before you can comment on or make changes to this bug.