Bug 213278 – IPC/Decoder.h and IPC/Encoder.h need every WTF::EnumTraits<> specialization included before they are included

• Component: Web Template Framework
• Status: NEW

[David Kilzer (:ddkilzer)]

IPC/Decoder.h and IPC/Encoder.h need every WTF::EnumTraits<> specialization #included before they are included.

See Bug 213224, Comment #12 for an example where <WebCore/ContextMenuItem.h> had to be included in IPC/Decoder.h and IPC/Encoder.h.

<https://bugs.webkit.org/show_bug.cgi?id=213224#c12>

I think this issue is exacerbated by unified sources since a prior source file (in the united source) may include IPC/Decoder.h and IPC/Encoder.h before a later WTF::EnumTraits<> is included.

[David Kilzer (:ddkilzer)]

I'm having to do this for the patch for Bug 213199 as well.

[Darin Adler]

It doesn’t really make logical sense that every specialization has to be included first. I think we have to more deeply understand why. Normally it’s when templates are expanded/used, not when they are included, that all the things they call have to be present. We probably have to get a more solid understanding of what creates this dependency.

We may change isValidEnum from a namespace-level function template to something different. Normally functions are specialized with overloading rather than specializing a template. That may be part of the problem here. Perhaps we can overload isValidEnum rather than specializing an isValidEnum function template. Another possibility is to use the traits pattern and have EnumValidityCheck be a class template with a static member function isValid. Not sure this solves the problem, but I think it might.

[David Kilzer (:ddkilzer)]

(In reply to David Kilzer (:ddkilzer) from comment #1)
> I'm having to do this for the patch for Bug 213199 as well.

I was wrong about this--ran into a different issue.

[David Kilzer (:ddkilzer)]

(In reply to Darin Adler from comment #2)
> It doesn’t really make logical sense that every specialization has to be
> included first. I think we have to more deeply understand why. Normally it’s
> when templates are expanded/used, not when they are included, that all the
> things they call have to be present. We probably have to get a more solid
> understanding of what creates this dependency.
> 
> We may change isValidEnum from a namespace-level function template to
> something different. Normally functions are specialized with overloading
> rather than specializing a template. That may be part of the problem here.
> Perhaps we can overload isValidEnum rather than specializing an isValidEnum
> function template. Another possibility is to use the traits pattern and have
> EnumValidityCheck be a class template with a static member function isValid.
> Not sure this solves the problem, but I think it might.

Okay, using pre-processed source, I've verified that if I don't #include <WebCore/ContextMenuItem.h> in Decoder.h/Encoder.h, then the specialized isValidEnum() function is included after it's needed in Decoder.h/Encoder.h when building Source/WebKit/Shared/WebContextMenuItemData.cpp.

I'm not sure how to turn your suggestion into code, though. I tried a first pass, but it seems much more complex than it should be (I didn't even try to compile it), and I don't see how it solves the #include order issue.  I'm probably doing it wrong, and figuring it out is going to take me much longer than it should to get it correct/working.  (I suspect my time could best be spent on other bugs, despite wanting to learn how to do this.)

This is what I wrote:


template<>
struct EnumValueChecker<false> {
    template<typename E, typename T>
    static constexpr bool isValid(T t)
    {
        static_assert(sizeof(T) >= sizeof(std::underlying_type_t<E>), "Integral type must be at least the size of the underlying enum type");

        return EnumValueChecker<T, typename EnumTraits<E>::values>::isValidEnum(t);
    }
};

template<>
struct EnumValueChecker<false> {
    template<typename E, typename T = bool>
    static constexpr bool isValid(T t)
    {
        return !t || t == 1;
    }
};

template<typename E, typename T>
constexpr bool isValidEnum(T t)
{
    return EnumValidityCheck<HasCustomIsValidEnum<E>::value>::isValid<E>(t);
}


I still think it requires an early #include, though.

[Darin Adler]

(In reply to David Kilzer (:ddkilzer) from comment #4)
> Okay, using pre-processed source, I've verified that if I don't #include
> <WebCore/ContextMenuItem.h> in Decoder.h/Encoder.h, then the specialized
> isValidEnum() function is included after it's needed in Decoder.h/Encoder.h
> when building Source/WebKit/Shared/WebContextMenuItemData.cpp.

We need to look deeper into *this*. Typically you don’t need things that templates depend on until you instantiate the template, so it doesn’t make logical sense that the order of includes would matter if these things in Decoder.h and Encoder.h are templates. Before we make any changes, we need to figure out more precisely exactly what goes wrong when ContextMenuItem.h is included after the templates, but before the templates are instantiated because they are used.

[David Kilzer (:ddkilzer)]

Created attachment 402580 [details]
Reproduce the build failure

This patch reproduces the build failure after removing #include <WebCore/ContextMenuItem.h> from Decoder.h/Encoder.h and adding it to Source/WebKit/Shared/WebContextMenuItemData.cpp.  (Adding it to WebContextMenuItemData.cpp doesn't actually make any difference, so this change could also be left out.  It's what I would expect to work, but doesn't.)

The call to WTF::isValidEnum<E>() when wants the template specialization defined is happening in Encoder.h (and Decoder.h), but both of those headers are included by an earlier *.cpp file in UnifiedSource24.cpp for WebKit.

So one solution might be to remove WebContextMenuItemData.cpp as supporting Unified Sources.  I have not tried that.