Bug 213616 – Non unified build fixes, midsummer 2020 edition

• Component: Tools / Tests
• Status: RESOLVED FIXED

[Adrian Perez]

Because there should not be a good midsummer without a good
WebKit patch — and some sauna 🧖‍♂️️

[Adrian Perez]

Created attachment 402805 [details]
Patch

[Mark Lam]

Comment on attachment 402805 [details]
Patch

View in context: https://bugs.webkit.org/attachment.cgi?id=402805&action=review

> Source/JavaScriptCore/ChangeLog:3
> +        Non unified build fixes, midsummer 20202 edition

You mean 2020, right?

[Adrian Perez]

(In reply to Mark Lam from comment #2)
> Comment on attachment 402805 [details]
> Patch
> 
> View in context:
> https://bugs.webkit.org/attachment.cgi?id=402805&action=review
> 
> > Source/JavaScriptCore/ChangeLog:3
> > +        Non unified build fixes, midsummer 20202 edition
> 
> You mean 2020, right?

Most certainly, yes O:-)

[Adrian Perez]

Created attachment 402814 [details]
Patch for landing

[Mark Lam]

Comment on attachment 402814 [details]
Patch for landing

View in context: https://bugs.webkit.org/attachment.cgi?id=402814&action=review

> Source/JavaScriptCore/b3/air/AirTmpWidth.h:31
> +#include "AirTmpInlines.h"

We should never #include an Inlines.h in a .h.  This is the wrong way to fix this issue.

[Adrian Perez]

(In reply to Mark Lam from comment #5)
> Comment on attachment 402814 [details]
> Patch for landing
> 
> View in context:
> https://bugs.webkit.org/attachment.cgi?id=402814&action=review
> 
> > Source/JavaScriptCore/b3/air/AirTmpWidth.h:31
> > +#include "AirTmpInlines.h"
> 
> We should never #include an Inlines.h in a .h.  This is the wrong way to fix
> this issue.

I'll see what I can do about this, thanks for commenting!

[Adrian Perez]

Created attachment 403379 [details]
Patch

[Adrian Perez]

It would be nice if Mark (or anyone else with JSC-fu) could please take
a look and tell if the approach of moving the function from AirTmpWidth.h
which used the inlines into the .cpp file and including AirTmpInlines.h
from there is correct — the rest are the usual boring fixes for non-unified
builds :]

Thanks in advance!

[Yusuke Suzuki]

Comment on attachment 403379 [details]
Patch

View in context: https://bugs.webkit.org/attachment.cgi?id=403379&action=review

> Source/JavaScriptCore/b3/air/AirTmpWidth.h:-116
> -    Widths& widths(Tmp tmp)
> -    {
> -        if (tmp.isGP()) {
> -            unsigned index = AbsoluteTmpMapper<GP>::absoluteIndex(tmp);
> -            ASSERT(index < m_widthGP.size());
> -            return m_widthGP[index];
> -        }
> -        unsigned index = AbsoluteTmpMapper<FP>::absoluteIndex(tmp);
> -        ASSERT(index < m_widthFP.size());
> -        return m_widthFP[index];
> -    }

Please keep it inline function :)

[Darin Adler]

Comment on attachment 403379 [details]
Patch

View in context: https://bugs.webkit.org/attachment.cgi?id=403379&action=review

> Source/WTF/wtf/text/StringParsingBuffer.h:31
>  #include <wtf/Forward.h>
>  #include <wtf/text/LChar.h>
>  #include <wtf/text/StringCommon.h>
> +#include <wtf/text/StringView.h>

Adding StringView.h allows us to remove some or all of the other includes above. Could you do that please?

> Source/WebCore/html/parser/ParsingUtilities.h:33
> -#include <wtf/text/StringCommon.h>
> +#include <wtf/text/StringParsingBuffer.h>

Is this truly needed? I would have expected that a forward declaration of StringParsingBuffer would suffice. And we could perhaps add that to Forward.h?

> Source/WebCore/platform/graphics/ColorSerialization.cpp:29
> +#include "Color.h"

Adding Color.h should allow us to remove some of the includes below.

> Source/WebCore/platform/graphics/ColorSerialization.h:28
> +#include <wtf/text/WTFString.h>

Should include Forward.h instead of WTFString.h.

[Adrian Perez]

(In reply to Darin Adler from comment #10)
> Comment on attachment 403379 [details]
> Patch
> 
> View in context:
> https://bugs.webkit.org/attachment.cgi?id=403379&action=review
> 
> > Source/WTF/wtf/text/StringParsingBuffer.h:31
> >  #include <wtf/Forward.h>
> >  #include <wtf/text/LChar.h>
> >  #include <wtf/text/StringCommon.h>
> > +#include <wtf/text/StringView.h>
> 
> Adding StringView.h allows us to remove some or all of the other includes
> above. Could you do that please?

Sure thing.

> > Source/WebCore/html/parser/ParsingUtilities.h:33
> > -#include <wtf/text/StringCommon.h>
> > +#include <wtf/text/StringParsingBuffer.h>
> 
> Is this truly needed? I would have expected that a forward declaration of
> StringParsingBuffer would suffice. And we could perhaps add that to
> Forward.h?

The following seems to work—I'll look into moving the forward declaration
into wtf/Forward.h, then:

diff --git a/Source/WebCore/html/parser/ParsingUtilities.h b/Source/WebCore/html/parser/ParsingUtilities.h
index e853ee7881f..a37a9171b7e 100644
--- a/Source/WebCore/html/parser/ParsingUtilities.h
+++ b/Source/WebCore/html/parser/ParsingUtilities.h
@@ -30,10 +30,18 @@
 
 #pragma once
 
-#include <wtf/text/StringParsingBuffer.h>
+#include <wtf/text/StringCommon.h>
+
+namespace WTF {
+
+template <typename T> class StringParsingBuffer;
+
+}; // namespace WTF
 
 namespace WebCore {
 
+using WTF::StringParsingBuffer;
+
 template<typename CharacterType> inline bool isNotASCIISpace(CharacterType c)
 {
     return !isASCIISpace(c);


> > Source/WebCore/platform/graphics/ColorSerialization.cpp:29
> > +#include "Color.h"
> 
> Adding Color.h should allow us to remove some of the includes below.

Aye, will try.

> > Source/WebCore/platform/graphics/ColorSerialization.h:28
> > +#include <wtf/text/WTFString.h>
> 
> Should include Forward.h instead of WTFString.h.

Okay, I'll try this as well.

Thanks for the comments!

[Adrian Perez]

(In reply to Yusuke Suzuki from comment #9)
> Comment on attachment 403379 [details]
> Patch
> 
> View in context:
> https://bugs.webkit.org/attachment.cgi?id=403379&action=review
> 
> > Source/JavaScriptCore/b3/air/AirTmpWidth.h:-116
> > -    Widths& widths(Tmp tmp)
> > -    {
> > -        if (tmp.isGP()) {
> > -            unsigned index = AbsoluteTmpMapper<GP>::absoluteIndex(tmp);
> > -            ASSERT(index < m_widthGP.size());
> > -            return m_widthGP[index];
> > -        }
> > -        unsigned index = AbsoluteTmpMapper<FP>::absoluteIndex(tmp);
> > -        ASSERT(index < m_widthFP.size());
> > -        return m_widthFP[index];
> > -    }
> 
> Please keep it inline function :)

If we keep it inline in AirTmpWidth.h, then we need to include
AirTmpInlines.h from there, and Mark advised against doing that.
Is there some other way of avoiding including AirTmpInlines.h _and_
also keeping the definition there? Any ideas are welcome :)

[Yusuke Suzuki]

Comment on attachment 403379 [details]
Patch

View in context: https://bugs.webkit.org/attachment.cgi?id=403379&action=review

>>> Source/JavaScriptCore/b3/air/AirTmpWidth.h:-116
>>> -    }
>> 
>> Please keep it inline function :)
> 
> If we keep it inline in AirTmpWidth.h, then we need to include
> AirTmpInlines.h from there, and Mark advised against doing that.
> Is there some other way of avoiding including AirTmpInlines.h _and_
> also keeping the definition there? Any ideas are welcome :)

Adding AirTmpWidthInlines.h and move it to that with `inline`, and include it if this function is used :)

[Adrian Perez]

Created attachment 404753 [details]
Patch

[Mark Lam]

Comment on attachment 404753 [details]
Patch

View in context: https://bugs.webkit.org/attachment.cgi?id=404753&action=review

> Source/JavaScriptCore/b3/air/AirTmpInlines.h:211
> +inline TmpWidth::Widths& TmpWidth::widths(Tmp tmp)
> +{
> +    if (tmp.isGP()) {
> +        unsigned index = AbsoluteTmpMapper<GP>::absoluteIndex(tmp);
> +        ASSERT(index < m_widthGP.size());
> +        return m_widthGP[index];
> +    }
> +    unsigned index = AbsoluteTmpMapper<FP>::absoluteIndex(tmp);
> +    ASSERT(index < m_widthFP.size());
> +    return m_widthFP[index];
> +}
> +

Given that there's a AirTmpWidth.h and a AirTmpWidth.cpp, this function should be in a AirTmpWidthInlines.h, not AirTmpInlines.h.

[Adrian Perez]

(In reply to Mark Lam from comment #15)
> Comment on attachment 404753 [details]
> Patch
> 
> View in context:
> https://bugs.webkit.org/attachment.cgi?id=404753&action=review
> 
> > Source/JavaScriptCore/b3/air/AirTmpInlines.h:211
> > +inline TmpWidth::Widths& TmpWidth::widths(Tmp tmp)
> > +{
> > +    if (tmp.isGP()) {
> > +        unsigned index = AbsoluteTmpMapper<GP>::absoluteIndex(tmp);
> > +        ASSERT(index < m_widthGP.size());
> > +        return m_widthGP[index];
> > +    }
> > +    unsigned index = AbsoluteTmpMapper<FP>::absoluteIndex(tmp);
> > +    ASSERT(index < m_widthFP.size());
> > +    return m_widthFP[index];
> > +}
> > +
> 
> Given that there's a AirTmpWidth.h and a AirTmpWidth.cpp, this function
> should be in a AirTmpWidthInlines.h, not AirTmpInlines.h.

Aye, will do, thanks for the feedback!

[Adrian Perez]

Created attachment 404768 [details]
Patch

[EWS]

Committed r264630: <https://trac.webkit.org/changeset/264630>

All reviewed patches have been landed. Closing bug and clearing flags on attachment 404768 [details].

[Radar WebKit Bug Importer]

<rdar://problem/65853702>