Applying attachment 349536 [details], I get the errors below on macOS and iOS ports. Since attachment 349536 [details] is unrelated to font stuff I suspect this is one more instance of UnifiedBuild rotating. A workaround is to move the implementation from the header to the cpp file. In file included from /Users/fred/WebKit/WebKitBuild/Debug/DerivedSources/WebCore/unified-sources/UnifiedSource321.cpp:8: ./platform/graphics/FontTaggedSettings.cpp:36:31: error: explicit specialization of 'hash' after instantiation unsigned FontFeatureSettings::hash() const ^ In file included from /Users/fred/WebKit/WebKitBuild/Debug/DerivedSources/WebCore/unified-sources/UnifiedSource321.cpp:1: In file included from ./platform/graphics/FontCascadeFonts.cpp:32: ./platform/graphics/FontCache.h:117:38: note: implicit instantiation first required here hasher.add(m_featureSettings.hash()); ^ In file included from /Users/fred/WebKit/WebKitBuild/Debug/DerivedSources/WebCore/unified-sources/UnifiedSource321.cpp:8: ./platform/graphics/FontTaggedSettings.cpp:48:33: error: explicit specialization of 'hash' after instantiation unsigned FontVariationSettings::hash() const ^ In file included from /Users/fred/WebKit/WebKitBuild/Debug/DerivedSources/WebCore/unified-sources/UnifiedSource321.cpp:1: In file included from ./platform/graphics/FontCascadeFonts.cpp:32: ./platform/graphics/FontCache.h:119:40: note: implicit instantiation first required here hasher.add(m_variationSettings.hash()); ^ 2 errors generated.
Created attachment 349537 [details] Patch (Move FontDescriptionKey::computeHash into the corresponding implementation file + ALWAYS_INLINE tag)
(In reply to Myles C. Maxfield from comment #15) > Comment on attachment 349552 [details] > Patch > > We often inline functions for performance, and fonts are used pretty often. > Is this a performance regression? ALWAYS_INLINE failed to build.
Created attachment 349566 [details] Patch (Move FontDescriptionKey::computeHash into the corresponding implementation file + inline tag)
Comment on attachment 349566 [details] Patch (Move FontDescriptionKey::computeHash into the corresponding implementation file + inline tag) View in context: https://bugs.webkit.org/attachment.cgi?id=349566&action=review > Source/WebCore/platform/graphics/FontCache.h:107 > + inline unsigned computeHash() const; This "inline" doesn't do anything any more. Is computeHash only used from within FontCache.cpp?
For the record, my initial attempt was in https://bugs.webkit.org/show_bug.cgi?id=185087#c14
Just to complete the description of the bug, the unified build content is: #include "platform/graphics/FontCascadeFonts.cpp" #include "platform/graphics/FontDescription.cpp" #include "platform/graphics/FontFamilySpecificationNull.cpp" #include "platform/graphics/FontGenericFamilies.cpp" #include "platform/graphics/FontPlatformData.cpp" #include "platform/graphics/FontRanges.cpp" #include "platform/graphics/FontSelectionAlgorithm.cpp" #include "platform/graphics/FontTaggedSettings.cpp" So the implicit instantiation happens in FontCache.h (included by FontCascadeFonts.cpp) before the explicit specialization (in FontTaggedSettings.cpp).
Created attachment 349651 [details] Patch (Move FontDescriptionKey::computeHash into the corresponding implementation file) This is attachment 349552 [details] (attached to the wrong bug) which did pass EWS but does not do any inlining.
A template function should be defined in header, not in a cpp file. FontFeatureSettings::hash() should be defined in FontTaggedSettings class definition.
(In reply to Fujii Hironori from comment #8) > A template function should be defined in header, not in a cpp file. > FontFeatureSettings::hash() should be defined in FontTaggedSettings class > definition. You're absolutely right. We should move FontFeatureSettings::hash and FontVariationSettings::hash to a header.
Created attachment 349755 [details] Patch (move template functions to header)
(In reply to Fujii Hironori from comment #8) > A template function should be defined in header, not in a cpp file. > FontFeatureSettings::hash() should be defined in FontTaggedSettings class > definition. (In reply to Alex Christensen from comment #9) > (In reply to Fujii Hironori from comment #8) > > A template function should be defined in header, not in a cpp file. > > FontFeatureSettings::hash() should be defined in FontTaggedSettings class > > definition. > > You're absolutely right. We should move FontFeatureSettings::hash and > FontVariationSettings::hash to a header. Thanks for the suggestion. I tried that in attachment 349755 [details] but as you can see linking fails with duplicate symbols. I can probably avoid this by setting the functions as inline but anyway I realize this is not relevant here. These two functions are already fully specialized so they are not template functions! The issue in comment 0 is "Explicit specialization must be declared before the first use that would cause implicit instantiation, in every translation unit where such use occurs" [1]. The problem is that unified build may put FontCache.h and FontTaggedSettings.cpp in the same unit. This can be avoided by moving FontDescriptionKey::computeHash to FontCache.cpp as done in attachment 349651 [details] but it could really still be possible that unified build puts FontCache.cpp and FontTaggedSettings.cpp in the same unit... So my guess is that the proper solution is to isolate FontTaggedSettings.cpp as suggested by Michael on webkit-dev. [1] https://en.cppreference.com/w/cpp/language/template_specialization
Created attachment 349758 [details] Patch (declare fully specialized functions in FontTaggedSetting.h)
Comment on attachment 349758 [details] Patch (declare fully specialized functions in FontTaggedSetting.h) Clearing flags on attachment: 349758 Committed r236013: <https://trac.webkit.org/changeset/236013>
All reviewed patches have been landed. Closing bug.
<rdar://problem/44463595>