Summary: | [SVG -> OTF Converter] Make Placeholder a move-only type | ||||||||||
---|---|---|---|---|---|---|---|---|---|---|---|
Product: | WebKit | Reporter: | Myles C. Maxfield <mmaxfield> | ||||||||
Component: | New Bugs | Assignee: | Myles C. Maxfield <mmaxfield> | ||||||||
Status: | RESOLVED FIXED | ||||||||||
Severity: | Normal | CC: | commit-queue, darin, d-r, fmalita, gyuyoung.kim, mitz, pdr, schenney, sergio | ||||||||
Priority: | P2 | ||||||||||
Version: | 528+ (Nightly build) | ||||||||||
Hardware: | Unspecified | ||||||||||
OS: | Unspecified | ||||||||||
Attachments: |
|
Description
Myles C. Maxfield
2014-12-22 11:38:02 PST
Created attachment 243631 [details]
Patch
I'm not sure if there's a better way to do this other than a movedOutOf boolean. Created attachment 243636 [details]
Patch
Comment on attachment 243636 [details] Patch View in context: https://bugs.webkit.org/attachment.cgi?id=243636&action=review > Source/WebCore/svg/SVGToOTFFontConversion.cpp:86 > + , m_movedOutOf(false) This initialization should be !ASSERT_DISABLED only. But it should be omitted entirely (see below). > Source/WebCore/svg/SVGToOTFFontConversion.cpp:103 > +#if !ASSERT_DISABLED > + , m_written(other.m_written) > +#endif Doesn’t seem right to me. We should ASSERT(other.m_written) and just initialize m_written to true. > Source/WebCore/svg/SVGToOTFFontConversion.cpp:106 > + other.m_movedOutOf = true; Should only compile this when !ASSERT_DISABLED. > Source/WebCore/svg/SVGToOTFFontConversion.cpp:123 > - ASSERT(m_written); > + ASSERT(m_movedOutOf || m_written); Change doesn’t make sense to me. No reason to check m_movedOutOf here that I can see. > Source/WebCore/svg/SVGToOTFFontConversion.cpp:130 > + unsigned m_movedOutOf : 1; This should be !ASSERT_DISABLED only. This should be bool, not a bit field, because it’s !ASSERT_DISABLED only and size not all that important. This should be initialized here to cut down on initialization in constructors: bool m_movedOutOf { false }; > Source/WebCore/svg/SVGToOTFFontConversion.cpp:132 > + unsigned m_written : 1; This should be bool, not a bit field, because it’s !ASSERT_DISABLED only and size not all that important. This should be initialized here to cut down on initialization in constructors: bool m_written { false }; Created attachment 243678 [details]
Patch
Comment on attachment 243636 [details] Patch View in context: https://bugs.webkit.org/attachment.cgi?id=243636&action=review >> Source/WebCore/svg/SVGToOTFFontConversion.cpp:86 >> + , m_movedOutOf(false) > > This initialization should be !ASSERT_DISABLED only. But it should be omitted entirely (see below). I've removed this variable, and renamed m_written to m_active, because a Placeholder which has been moved from but hasn't written anything yet should be marked as inactive. Populate() will then set m_active to false, and the destructor will ASSERT(!m_active); Comment on attachment 243678 [details] Patch View in context: https://bugs.webkit.org/attachment.cgi?id=243678&action=review > Source/WebCore/svg/SVGToOTFFontConversion.cpp:90 > + Placeholder(const Placeholder&) = delete; Implementing a move constructor is enough to not make it have a copy constructor, so this isn't needed. Committed r177688: <http://trac.webkit.org/changeset/177688> |