RESOLVED FIXED 189541
Build error in FontDescriptionKey::computeHash when compiling FontTaggedSettings and FontCascadeFonts together 
https://bugs.webkit.org/show_bug.cgi?id=189541
Summary Build error in FontDescriptionKey::computeHash when compiling FontTaggedSetti...
Frédéric Wang (:fredw)
Reported 2018-09-12 02:50:08 PDT
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.
Attachments
Patch (Move FontDescriptionKey::computeHash into the corresponding implementation file + ALWAYS_INLINE tag) (3.67 KB, patch)
2018-09-12 03:09 PDT, Frédéric Wang (:fredw) 		no flags
DetailsFormatted DiffDiff
Patch (Move FontDescriptionKey::computeHash into the corresponding implementation file + inline tag) (3.48 KB, patch)
2018-09-12 11:37 PDT, Frédéric Wang (:fredw) 		no flags
DetailsFormatted DiffDiff
Patch (Move FontDescriptionKey::computeHash into the corresponding implementation file) (3.43 KB, patch)
2018-09-13 03:58 PDT, Frédéric Wang (:fredw) 		no flags
DetailsFormatted DiffDiff
Patch (move template functions to header) (2.71 KB, patch)
2018-09-14 02:38 PDT, Frédéric Wang (:fredw) 		no flags
DetailsFormatted DiffDiff
Patch (declare fully specialized functions in FontTaggedSetting.h) (2.51 KB, patch)
2018-09-14 07:36 PDT, Frédéric Wang (:fredw) 		no flags
DetailsFormatted DiffDiff
View All Add attachment proposed patch, testcase, etc.
Frédéric Wang (:fredw)
Comment 1 2018-09-12 03:09:22 PDT
Created attachment 349537 [details] Patch (Move FontDescriptionKey::computeHash into the corresponding implementation file + ALWAYS_INLINE tag)
Frédéric Wang (:fredw)
Comment 2 2018-09-12 11:32:30 PDT
(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.
Frédéric Wang (:fredw)
Comment 3 2018-09-12 11:37:37 PDT
Created attachment 349566 [details] Patch (Move FontDescriptionKey::computeHash into the corresponding implementation file + inline tag)
Alex Christensen
Comment 4 2018-09-12 13:21:01 PDT
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?
Frédéric Wang (:fredw)
Comment 5 2018-09-12 21:39:15 PDT
For the record, my initial attempt was in https://bugs.webkit.org/show_bug.cgi?id=185087#c14
Frédéric Wang (:fredw)
Comment 6 2018-09-13 03:05:01 PDT
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).
Frédéric Wang (:fredw)
Comment 7 2018-09-13 03:58:45 PDT
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.
Fujii Hironori
Comment 8 2018-09-13 07:07:22 PDT
A template function should be defined in header, not in a cpp file. FontFeatureSettings::hash() should be defined in FontTaggedSettings class definition.
Alex Christensen
Comment 9 2018-09-13 09:09:16 PDT
(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.
Frédéric Wang (:fredw)
Comment 10 2018-09-14 02:38:30 PDT
Created attachment 349755 [details] Patch (move template functions to header)
Frédéric Wang (:fredw)
Comment 11 2018-09-14 06:45:26 PDT
(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
Frédéric Wang (:fredw)
Comment 12 2018-09-14 07:36:07 PDT
Created attachment 349758 [details] Patch (declare fully specialized functions in FontTaggedSetting.h)
WebKit Commit Bot
Comment 13 2018-09-14 11:33:53 PDT
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>
WebKit Commit Bot
Comment 14 2018-09-14 11:33:55 PDT
All reviewed patches have been landed. Closing bug.
Radar WebKit Bug Importer
Comment 15 2018-09-14 11:34:32 PDT
<rdar://problem/44463595>
Note You need to log in before you can comment on or make changes to this bug.