Bug 139870 – [SVG -> OTF Converter] Make Placeholder a move-only type

• Status: RESOLVED
• Resolution: FIXED
• Component: New Bugs

[Myles C. Maxfield]

[SVG -> OTF Converter] Make Placeholder a move-only type

[Myles C. Maxfield]

Created attachment 243631 [details]
Patch

[Myles C. Maxfield]

I'm not sure if there's a better way to do this other than a movedOutOf boolean.

[Myles C. Maxfield]

Created attachment 243636 [details]
Patch

[Darin Adler]

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 };

[Myles C. Maxfield]

Created attachment 243678 [details]
Patch

[Myles C. Maxfield]

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);

[Anders Carlsson]

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.

[Myles C. Maxfield]

Committed r177688: <http://trac.webkit.org/changeset/177688>