Closed Bug 1157656 Opened 5 years ago Closed 5 years ago

Initialize all gfxGlyphExtents::HashEntry members in the constructor

Categories

(Core :: Graphics, defect, trivial)

defect
Not set
trivial

Tracking

()

RESOLVED INVALID

People

(Reporter: nical, Assigned: robin.moussu+git, Mentored)

References

(Blocks 1 open bug)

Details

(Keywords: coverity)

The code is currently correct since in the only place we create a HashEntry half initialized we set the other members manually, but there doesn't seem to be a reason to not initialize all members in the constructor, and this would get rid of some coverity warnings.
I haven't looked into just what we'd need to change for this, but it would make me sad if it results in first zero-initializing the members, only to overwrite them with the real values.
Just need to initialize the members in the constructor here instead of doing it manually a few lines below: https://hg.mozilla.org/mozilla-central/file/a5af73b32ac8/gfx/thebes/gfxGlyphExtents.cpp#l133
Keywords: coverity
Je souhaites m'occuper de ce bug [comme dit sur irc].
(In reply to robin.moussu+git@gmail.com from comment #3)
> Je souhaites m'occuper de ce bug [comme dit sur irc].

Cool (let's do it in English on bugzilla, though).
Assignee: nobody → robin.moussu+git
> Cool (let's do it in English on bugzilla, though).

Sure, sorry for being messy :)
So, I have a class `nsUint32HashKey`, with some public attribute
> public:
> float x, y, width, height;
and a constructor
> explicit HashEntry(KeyTypePointer aPtr) : nsUint32HashKey(aPtr) {}
which leave those attributes non-initialised.

The function 
> void gfxGlyphExtents::SetTightGlyphExtents(uint32_t aGlyphID, const gfxRect& aExtentsAppUnits)
Initialize those attributes.

So what I want to do is to merge the constructor and SetTightGlyphExtents() (so add the two parameters of SetTightGlyphExtents to the constructor, and remote SetTightGlyphExtents()), and set the attributes private. Am I right?

NB: SetTightGlyphExtentsvd is only use in gfx/thebes/gfxFont.cpp
edit: SetTightGlyphExtents() is a function of the parent of class `nsUint32HashKey`, so of course, I do not remove that function.
You can simply add the missing members to the constructor and SetTightGlyphExtents would look something like

> mThighGlyphExtents.PutEntry(HashEntry(aGlyphID,
>                                       aExtentsAppUnits.X(),
> [etc.]

No need to set the attributes private for now, we just want to shut coverity's uninitialized-member warnings up.
A night of sleep was a good thing.

I follow a bit the use graph of HashEntry. What I understood, is that an nsTHashTable<HashEntry> is created in GfxGlyphExtents. HashEntry are half-initialized. When we call GfxGlyphExtents.SetTightGlyphExtents(), they are set, and there is obvious link between the creation of the object to SetTightGlyphExtents().

So I think that the only think I can do is first zero-initializing the members of HashEntry, only to overwrite them with the real values in SetTightGlyphExtents().
@Nicolas Silva: Do you think that zero-initializing is a good thing ?
(In reply to robin.moussu+git@gmail.com from comment #9)
> A night of sleep was a good thing.
> 

Actually, sorry, I am the one who needs a good night of sleep. I read the code backwards (I thought we could also pass an entry as parameter of PutEntry but you only pass the key) and it would be wasteful to zero-initialize the entry and set the members right after that just to get rid of a benign coverity warning so I'll close the bug and mark the warning as false-positive.
Status: NEW → RESOLVED
Closed: 5 years ago
Resolution: --- → INVALID
You need to log in before you can comment on or make changes to this bug.