213278 2020-06-16 18:14:12 -0700 IPC/Decoder.h and IPC/Encoder.h need every WTF::EnumTraits<> specialization included before they are included 2020-06-23 12:00:11 -0700 1 1 1 Unclassified WebKit Web Template Framework Other Unspecified Unspecified NEW https://bugs.webkit.org/show_bug.cgi?id=213224 https://bugs.webkit.org/show_bug.cgi?id=213199 P2 Normal --- 1 ddkilzer webkit-unassigned achristensen darin oldest_to_newest 1663334 0 ddkilzer 2020-06-16 18:14:12 -0700 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. 1663336 1 ddkilzer 2020-06-16 18:15:24 -0700 I'm having to do this for the patch for Bug 213199 as well. 1663384 2 darin 2020-06-16 23:38:33 -0700 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. 1663750 3 ddkilzer 2020-06-17 18:44:02 -0700 (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. 1665279 4 ddkilzer 2020-06-22 21:11:46 -0700 (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. 1665428 5 darin 2020-06-23 09:57:27 -0700 (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. 1665485 6 402580 ddkilzer 2020-06-23 12:00:11 -0700 Created attachment 402580 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. 402580 2020-06-23 12:00:11 -0700 2020-06-23 12:00:11 -0700 Reproduce the build failure bug-213278-reproduce-build-failure.diff text/plain 1267 ddkilzer ZGlmZiAtLWdpdCBhL1NvdXJjZS9XZWJLaXQvUGxhdGZvcm0vSVBDL0RlY29kZXIuaCBiL1NvdXJj ZS9XZWJLaXQvUGxhdGZvcm0vSVBDL0RlY29kZXIuaAppbmRleCAzMDFmODgyMWU2NS4uMGExMzJi MmI4MDcgMTAwNjQ0Ci0tLSBhL1NvdXJjZS9XZWJLaXQvUGxhdGZvcm0vSVBDL0RlY29kZXIuaAor KysgYi9Tb3VyY2UvV2ViS2l0L1BsYXRmb3JtL0lQQy9EZWNvZGVyLmgKQEAgLTI5LDcgKzI5LDYg QEAKICNpbmNsdWRlICJBdHRhY2htZW50LmgiCiAjaW5jbHVkZSAiTWVzc2FnZU5hbWVzLmgiCiAj aW5jbHVkZSAiU3RyaW5nUmVmZXJlbmNlLmgiCi0jaW5jbHVkZSA8V2ViQ29yZS9Db250ZXh0TWVu dUl0ZW0uaD4KICNpbmNsdWRlIDx3dGYvT3B0aW9uU2V0Lmg+CiAjaW5jbHVkZSA8d3RmL1ZlY3Rv ci5oPgogCmRpZmYgLS1naXQgYS9Tb3VyY2UvV2ViS2l0L1BsYXRmb3JtL0lQQy9FbmNvZGVyLmgg Yi9Tb3VyY2UvV2ViS2l0L1BsYXRmb3JtL0lQQy9FbmNvZGVyLmgKaW5kZXggMTNmM2M0M2U0MTcu LmQzNGQyZWI3Nzk1IDEwMDY0NAotLS0gYS9Tb3VyY2UvV2ViS2l0L1BsYXRmb3JtL0lQQy9FbmNv ZGVyLmgKKysrIGIvU291cmNlL1dlYktpdC9QbGF0Zm9ybS9JUEMvRW5jb2Rlci5oCkBAIC0yOSw3 ICsyOSw2IEBACiAjaW5jbHVkZSAiQXR0YWNobWVudC5oIgogI2luY2x1ZGUgIk1lc3NhZ2VOYW1l cy5oIgogI2luY2x1ZGUgIlN0cmluZ1JlZmVyZW5jZS5oIgotI2luY2x1ZGUgPFdlYkNvcmUvQ29u dGV4dE1lbnVJdGVtLmg+CiAjaW5jbHVkZSA8d3RmL09wdGlvblNldC5oPgogI2luY2x1ZGUgPHd0 Zi9WZWN0b3IuaD4KIApkaWZmIC0tZ2l0IGEvU291cmNlL1dlYktpdC9TaGFyZWQvV2ViQ29udGV4 dE1lbnVJdGVtRGF0YS5jcHAgYi9Tb3VyY2UvV2ViS2l0L1NoYXJlZC9XZWJDb250ZXh0TWVudUl0 ZW1EYXRhLmNwcAppbmRleCA4OTQ2YzM1NTE1Mi4uY2JiYmI2NDg0YWEgMTAwNjQ0Ci0tLSBhL1Nv dXJjZS9XZWJLaXQvU2hhcmVkL1dlYkNvbnRleHRNZW51SXRlbURhdGEuY3BwCisrKyBiL1NvdXJj ZS9XZWJLaXQvU2hhcmVkL1dlYkNvbnRleHRNZW51SXRlbURhdGEuY3BwCkBAIC0zMyw2ICszMyw3 IEBACiAjaW5jbHVkZSAiQXJndW1lbnRDb2RlcnMuaCIKICNpbmNsdWRlIDx3dGYvdGV4dC9DU3Ry aW5nLmg+CiAjaW5jbHVkZSA8V2ViQ29yZS9Db250ZXh0TWVudS5oPgorI2luY2x1ZGUgPFdlYkNv cmUvQ29udGV4dE1lbnVJdGVtLmg+CiAKIG5hbWVzcGFjZSBXZWJLaXQgewogdXNpbmcgbmFtZXNw YWNlIFdlYkNvcmU7Cg==