Closed Bug 1494515 Opened Last year Closed Last year

Some static atom cleanups

Categories

(Core :: XPCOM, enhancement)

enhancement
Not set

Tracking

()

RESOLVED FIXED
mozilla64
Tracking Status
firefox64 --- fixed

People

(Reporter: njn, Assigned: njn)

Details

Attachments

(3 files)

These cleanups are possible due to previous changes, esp. bug 1482782 which combined all the static atom sources into nsGkAtoms.
nsGkAtoms.h has a big comment explaining how static atom definitions get
expanded by macros. This comment was very useful when there were multiple
sources of static atoms and their definitions used macros a lot. But bug
1482782 combined all the static atom sources into nsGkAtoms and removed a lot
of macro use. So now the comment is now something of a hindrance, duplicating
quite a bit of the code (and not entirely accurately).

This commit removes the big comment, and moves the still-useful parts inline
with the code. This makes things much easier to follow.

This commit also reformats some of the remaining macros so they are easier to
read.
Attachment #9012483 - Flags: review?(nfroyd)
Assignee: nobody → n.nethercote
Status: NEW → ASSIGNED
Static atom registration is a bit of a mess. NS_InitAtomTable() calls
nsGkAtoms::RegisterStaticAtoms(), which calls NS_RegisterStaticAtoms(); i.e. we
go from nsAtomTable.cpp to nsGkAtoms.cpp and back.

(And NS_RegisterStaticAtoms() is declared in a .cpp file, not a .h file!)

This commit makes NS_InitAtomTable() a friend of nsGkAtoms, so NS_InitAtomTable
can see nsGkAtoms's atoms array directly, thus removing the need for
NS_RegisterAtomTable() and nsGkAtoms::RegisterStaticAtoms().

This commit also removes an out-of-date part of a comment from XPCOMInit.cpp.
Attachment #9012484 - Flags: review?(nfroyd)
Presumably the commit that introduced `aStringOffset` could have removed this,
but overlooked it.
Attachment #9012485 - Flags: review?(nfroyd)
Attachment #9012483 - Flags: review?(nfroyd) → review+
Attachment #9012484 - Flags: review?(nfroyd) → review+
Comment on attachment 9012485 [details] [diff] [review]
Remove an unused argument from nsStaticAtom's constructor

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

::: xpcom/ds/nsAtom.h
@@ -107,5 @@
>    typedef mozilla::TrueType HasThreadSafeRefCnt;
>  
>  protected:
>    // Used by nsStaticAtom.
> -  constexpr nsAtom(const char16_t* aStr, uint32_t aLength, uint32_t aHash)

You would think one of our ever-zealous compiler warning settings would have complained about this.
Attachment #9012485 - Flags: review?(nfroyd) → review+
> You would think one of our ever-zealous compiler warning settings would have complained about this.

That requires -Wunused-parameter, which we currently don't enable. See bug 1232547 for details.
Pushed by nnethercote@mozilla.com:
https://hg.mozilla.org/integration/mozilla-inbound/rev/aab722576793
Rework the comments about static atoms. r=froydnj
https://hg.mozilla.org/integration/mozilla-inbound/rev/1a9268216cd7
Clean up static atom registration. r=froydnj
https://hg.mozilla.org/integration/mozilla-inbound/rev/cb1bd4097d6b
Remove an unused argument from nsStaticAtom's constructor. r=froydnj
You need to log in before you can comment on or make changes to this bug.