Bug 150109 – AtomicString.h cannot be imported by files compiled using ARC

• Component: Web Template Framework
• Status: RESOLVED WONTFIX

[Conrad Shultz]

In AtomicString.h, there is:

AtomicString::AtomicString(NSString* s) : m_string(AtomicStringImpl::add((CFStringRef)s)) {}

Toll-free bridging under ARC requires a bridge cast, which prevents AtomicString.h (and any headers which include it) from being imported by files compiled under ARC.

[Radar WebKit Bug Importer]

<rdar://problem/23101130>

[Conrad Shultz]

Created attachment 263045 [details]
Patch

[Alexey Proskuryakov]

My understanding was that we were moving away from using WTF outside of WebKit. HAs that changed?

[Remy Demarest]

Comment on attachment 263045 [details]
Patch

> Subversion Revision: 191003
> diff --git a/Source/WTF/ChangeLog b/Source/WTF/ChangeLog
> index ac74456731d92243073249a0485674f2c6443da7..b99507caff9cda1716438f41aec3774472294d61 100644
> --- a/Source/WTF/ChangeLog
> +++ b/Source/WTF/ChangeLog
> @@ -1,3 +1,16 @@
> [phone]  Conrad Shultz  <[e-mail]>
> +
> +        AtomicString.h cannot be imported by files compiled using ARC
> +        https://bugs.webkit.org/show_bug.cgi?id=150109
> +
> +        Reviewed by NOBODY (OOPS!).
> +
> +        The lack of a bridge cast when toll-free bridging prevents AtomicString.h (and any headers which include it)
> +        from being imported by files compiled under ARC.
> +
> +        * wtf/text/AtomicString.h:
> +        (WTF::AtomicString::AtomicString):
> +
>  2015-10-12  Andreas Kling  <[e-mail]>
>  
>          "A + B" with strings shouldn't copy if A or B is empty.
> diff --git a/Source/WTF/wtf/text/AtomicString.h b/Source/WTF/wtf/text/AtomicString.h
> index 9dc3afd58251cbe7352c0ca5c08a8a6a0396be98..9c3bbe721bfe85a18e7f23817269d4af7ec7801f 100644
> --- a/Source/WTF/wtf/text/AtomicString.h
> +++ b/Source/WTF/wtf/text/AtomicString.h
> @@ -292,7 +292,7 @@ inline AtomicString::AtomicString(CFStringRef s)
>  
>  #ifdef __OBJC__
>  inline AtomicString::AtomicString(NSString* s)
> -    : m_string(AtomicStringImpl::add((CFStringRef)s))
> +    : m_string(AtomicStringImpl::add((__bridge CFStringRef)s))

Do you need to guard this with __has_feature(objc_arc) ?

>  {
>  }
>  #endif

[Conrad Shultz]

(In reply to comment #3)
> My understanding was that we were moving away from using WTF outside of
> WebKit. HAs that changed?

Not as far as I know, but the problem is that this header is included by other headers, such as WTFString.h, which are in turn included by myriad headers throughout the stack. This therefore breaks compiling with ARC simply by including any of these headers, many of which are outside WTF.

[Conrad Shultz]

(In reply to comment #4)
> Comment on attachment 263045 [details]
> Patch
> 
> > Subversion Revision: 191003
> > diff --git a/Source/WTF/ChangeLog b/Source/WTF/ChangeLog
> > index ac74456731d92243073249a0485674f2c6443da7..b99507caff9cda1716438f41aec3774472294d61 100644
> > --- a/Source/WTF/ChangeLog
> > +++ b/Source/WTF/ChangeLog
> > @@ -1,3 +1,16 @@
> > [phone]  Conrad Shultz  <[e-mail]>
> > +
> > +        AtomicString.h cannot be imported by files compiled using ARC
> > +        https://bugs.webkit.org/show_bug.cgi?id=150109
> > +
> > +        Reviewed by NOBODY (OOPS!).
> > +
> > +        The lack of a bridge cast when toll-free bridging prevents AtomicString.h (and any headers which include it)
> > +        from being imported by files compiled under ARC.
> > +
> > +        * wtf/text/AtomicString.h:
> > +        (WTF::AtomicString::AtomicString):
> > +
> >  2015-10-12  Andreas Kling  <[e-mail]>
> >  
> >          "A + B" with strings shouldn't copy if A or B is empty.
> > diff --git a/Source/WTF/wtf/text/AtomicString.h b/Source/WTF/wtf/text/AtomicString.h
> > index 9dc3afd58251cbe7352c0ca5c08a8a6a0396be98..9c3bbe721bfe85a18e7f23817269d4af7ec7801f 100644
> > --- a/Source/WTF/wtf/text/AtomicString.h
> > +++ b/Source/WTF/wtf/text/AtomicString.h
> > @@ -292,7 +292,7 @@ inline AtomicString::AtomicString(CFStringRef s)
> >  
> >  #ifdef __OBJC__
> >  inline AtomicString::AtomicString(NSString* s)
> > -    : m_string(AtomicStringImpl::add((CFStringRef)s))
> > +    : m_string(AtomicStringImpl::add((__bridge CFStringRef)s))
> 
> Do you need to guard this with __has_feature(objc_arc) ?

Since this is inside an #ifdef __OBJC__, and all tool chains should be new enough to understand ARC, I don't think so. I see other places where __bridge is used without checking availability.

[Anders Carlsson]

(In reply to comment #5)
> (In reply to comment #3)
> > My understanding was that we were moving away from using WTF outside of
> > WebKit. HAs that changed?
> 
> Not as far as I know, but the problem is that this header is included by
> other headers, such as WTFString.h, which are in turn included by myriad
> headers throughout the stack. This therefore breaks compiling with ARC
> simply by including any of these headers, many of which are outside WTF.

We should make sure that any headers used by Safari do not end up including headers like WTFString.h. That's what the patch should do instead.

[Conrad Shultz]

(In reply to comment #7)
> (In reply to comment #5)
> > (In reply to comment #3)
> > > My understanding was that we were moving away from using WTF outside of
> > > WebKit. HAs that changed?
> > 
> > Not as far as I know, but the problem is that this header is included by
> > other headers, such as WTFString.h, which are in turn included by myriad
> > headers throughout the stack. This therefore breaks compiling with ARC
> > simply by including any of these headers, many of which are outside WTF.
> 
> We should make sure that any headers used by Safari do not end up including
> headers like WTFString.h. That's what the patch should do instead.

Alright, I'll NTBF this.

[Darin Adler]

I agree with Anders about the correct solution to the compiling problem, but I also like the idea of starting to future proof WebKit to get it ready to some day adopt ARC. Would not have minded adding this __bridge because of that.