Bug 238693 – Mark String(const char*) constructor as explicit

• Status: RESOLVED
• Resolution: FIXED
• Component: Web Template Framework

[Chris Dumez]

Mark String(const char*) constructor as explicit to encourage developers to use ASCIILiteral and the ""_s suffix more. This constructor should also not be called if the input may contain UTF-8 since it treats input as latin1.

[Chris Dumez]

Created attachment 456430 [details]
WIP Patch

[Chris Dumez]

Created attachment 456431 [details]
WIP Patch

[Chris Dumez]

Created attachment 456433 [details]
WIP Patch

[Chris Dumez]

Created attachment 456434 [details]
WIP Patch

[Chris Dumez]

Created attachment 456437 [details]
WIP Patch

[Chris Dumez]

Created attachment 456441 [details]
WIP Patch

[Chris Dumez]

Created attachment 456443 [details]
WIP Patch

[Chris Dumez]

Created attachment 456445 [details]
WIP Patch

[Chris Dumez]

Created attachment 456446 [details]
WIP Patch

[Chris Dumez]

Created attachment 456448 [details]
WIP Patch

[Chris Dumez]

Created attachment 456449 [details]
WIP Patch

[Chris Dumez]

Created attachment 456451 [details]
WIP Patch

[Chris Dumez]

Created attachment 456452 [details]
WIP Patch

[Darin Adler]

Comment on attachment 456452 [details]
WIP Patch

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

Super-great; just love this.

Given how many changes there still are here, I still think we might want another round or two of "prep" patch?

The only tricky parts of this patch are places where we add String { } and maybe Latin-1 is not what we want, and it’s still subtle. Would be better if it was named String::fromLatin1, and the places where we add String::fromUTF8 without checking, because Latin-1 conversion can’t fail, but UTF-8 conversion can.

> Source/WTF/wtf/posix/FileSystemPOSIX.cpp:246
> +    auto fsFile = fileSystemRepresentation(path);

Can fileSystemRepresentation return null when the string is non-null? Just wondering since the code we were deleting did have a check for null. I wonder if we should add checks for null at all the fileSystemRepresentation call sites.

> Source/WTF/wtf/text/WTFString.h:84
>      WTF_EXPORT_PRIVATE String(const LChar* characters);

As another bit of cleanup, not nearly as important as the ASCIILiteral work, I wonder if we really need this one any more; how common is a null-terminated LChar array?

> Source/WTF/wtf/text/WTFString.h:87
> +    // as the String(ASCIILiteral) is more efficient. Also note that this constructor treats input as latin1.

I would write Latin-1 rather than latin1, here and in the other comment above.

> Source/WTF/wtf/text/WTFString.h:89
> +    WTF_EXPORT_PRIVATE explicit String(const char* characters);

I think we should go further and rename this to String::fromLatin1 now that there are so many fewer call sites.

> Source/WebCore/Modules/mediastream/gstreamer/GStreamerMediaEndpoint.cpp:1100
> +        auto exc = Exception { OperationError, error ? String::fromUTF8(error->message) : "Unknown Error"_s };

In cases like this I always wonder about the "illegal UTF-8" case where String::fromUTF8 will return null. Obviously not a large concern, but a small simmering worry in the back of my mind.

> Source/WebCore/accessibility/ios/AXObjectCacheIOS.mm:47
> +    ASCIILiteral name = ASCIILiteral::null();

Why doesn’t ASCIILiteral default to null without explicit initialization?

> Source/WebCore/editing/ios/EditorIOS.mm:93
> +    ASCIILiteral newValue = ASCIILiteral::null();

Ditto.

> Source/WebCore/page/DebugPageOverlays.cpp:261
> +        ASCIILiteral key { ASCIILiteral::null() };
> +        ASCIILiteral name { ASCIILiteral::null() };

Same question.

> Source/WebKit/Shared/API/Cocoa/WKRemoteObjectCoder.mm:59
> +    if ([local compare:wire] ==  NSOrderedSame)

isEqualToString: should be used here instead of compare: since it’s more efficient.

Might also need to add a null check of wire, since I don’t think compare: or isEqualToString: work with nil.

> Source/WebKit/Shared/API/Cocoa/WKRemoteObjectCoder.mm:68
> +        char localType = [local characterAtIndex:i];
> +        char wireType = [wire characterAtIndex:i];

This chops the characters down to "char", which is only OK if we are guaranteed the strings contain no non-ASCII characters. Not new, but irritating. I know the people who wrote this originally think the use of strchr is clever, but I would write this:

    auto map = [](unichar character) -> unichar {
        switch (character) {
        case 'B':
            // `bool` and `signed char` are interchangeable.
            return 'c';
        default:
            return character;
        }
    };
    if (map(localType) != map(wireType))
        return false;

Very easy to add any other equivalencies in the same fashion.

[Chris Dumez]

Created attachment 456461 [details]
WIP Patch

[Chris Dumez]

(In reply to Darin Adler from comment #14)
> Comment on attachment 456452 [details]
> WIP Patch
> 
> View in context:
> https://bugs.webkit.org/attachment.cgi?id=456452&action=review
> 
> Super-great; just love this.
> 
> Given how many changes there still are here, I still think we might want
> another round or two of "prep" patch?

I was waiting to see how big the patch will get. Sadly, the remaining failures are for other ports so I am relying on EWS to find failures for me.
I am hoping to make the constructor explicit as soon as possible though. The longer I wait, the more likely people will introduce new code without ASCIILiteral.

> 
> The only tricky parts of this patch are places where we add String { } and
> maybe Latin-1 is not what we want, and it’s still subtle. Would be better if
> it was named String::fromLatin1, and the places where we add
> String::fromUTF8 without checking, because Latin-1 conversion can’t fail,
> but UTF-8 conversion can.
> 
> > Source/WTF/wtf/posix/FileSystemPOSIX.cpp:246
> > +    auto fsFile = fileSystemRepresentation(path);
> 
> Can fileSystemRepresentation return null when the string is non-null? Just
> wondering since the code we were deleting did have a check for null. I
> wonder if we should add checks for null at all the fileSystemRepresentation
> call sites.

Looking at the Cocoa implementation of fileSystemRepresentation(), I believe it is indeed possible for it to return a null CString() when provided a non-null String as input.

> 
> > Source/WTF/wtf/text/WTFString.h:84
> >      WTF_EXPORT_PRIVATE String(const LChar* characters);
> 
> As another bit of cleanup, not nearly as important as the ASCIILiteral work,
> I wonder if we really need this one any more; how common is a
> null-terminated LChar array?

Agreed, I can look into that in a follow-up. Similarly, AtomString has an implicit constructor from `const char*` too that should probably be explicit.

> 
> > Source/WTF/wtf/text/WTFString.h:87
> > +    // as the String(ASCIILiteral) is more efficient. Also note that this constructor treats input as latin1.
> 
> I would write Latin-1 rather than latin1, here and in the other comment
> above.

Will fix.

> 
> > Source/WTF/wtf/text/WTFString.h:89
> > +    WTF_EXPORT_PRIVATE explicit String(const char* characters);
> 
> I think we should go further and rename this to String::fromLatin1 now that
> there are so many fewer call sites.

I was wondering about the same thing. I like it but I'd probably want to do this separately as this is going to be a fairly large (though mechanical) change.

> 
> > Source/WebCore/Modules/mediastream/gstreamer/GStreamerMediaEndpoint.cpp:1100
> > +        auto exc = Exception { OperationError, error ? String::fromUTF8(error->message) : "Unknown Error"_s };
> 
> In cases like this I always wonder about the "illegal UTF-8" case where
> String::fromUTF8 will return null. Obviously not a large concern, but a
> small simmering worry in the back of my mind.
> 
> > Source/WebCore/accessibility/ios/AXObjectCacheIOS.mm:47
> > +    ASCIILiteral name = ASCIILiteral::null();
> 
> Why doesn’t ASCIILiteral default to null without explicit initialization?

Good question, I think we totally should have a default constructor. It would make ASCIILiteral a bit easier to use.

> 
> > Source/WebCore/editing/ios/EditorIOS.mm:93
> > +    ASCIILiteral newValue = ASCIILiteral::null();
> 
> Ditto.
> 
> > Source/WebCore/page/DebugPageOverlays.cpp:261
> > +        ASCIILiteral key { ASCIILiteral::null() };
> > +        ASCIILiteral name { ASCIILiteral::null() };
> 
> Same question.
> 
> > Source/WebKit/Shared/API/Cocoa/WKRemoteObjectCoder.mm:59
> > +    if ([local compare:wire] ==  NSOrderedSame)
> 
> isEqualToString: should be used here instead of compare: since it’s more
> efficient.

Good to know, will update.

> 
> Might also need to add a null check of wire, since I don’t think compare: or
> isEqualToString: work with nil.

Ok.

> 
> > Source/WebKit/Shared/API/Cocoa/WKRemoteObjectCoder.mm:68
> > +        char localType = [local characterAtIndex:i];
> > +        char wireType = [wire characterAtIndex:i];
> 
> This chops the characters down to "char", which is only OK if we are
> guaranteed the strings contain no non-ASCII characters. Not new, but
> irritating. I know the people who wrote this originally think the use of
> strchr is clever, but I would write this:
> 
>     auto map = [](unichar character) -> unichar {
>         switch (character) {
>         case 'B':
>             // `bool` and `signed char` are interchangeable.
>             return 'c';
>         default:
>             return character;
>         }
>     };
>     if (map(localType) != map(wireType))
>         return false;
> 
> Very easy to add any other equivalencies in the same fashion.

[Chris Dumez]

Created attachment 456462 [details]
WIP Patch

[Chris Dumez]

Created attachment 456466 [details]
WIP Patch

[Chris Dumez]

Created attachment 456467 [details]
WIP Patch

[Chris Dumez]

Created attachment 456470 [details]
WIP Patch

[Chris Dumez]

Created attachment 456471 [details]
WIP Patch

[Chris Dumez]

Created attachment 456473 [details]
WIP Patch

[Chris Dumez]

Created attachment 456475 [details]
WIP Patch

[Chris Dumez]

Created attachment 456476 [details]
WIP Patch

[Chris Dumez]

Created attachment 456478 [details]
WIP Patch

[Chris Dumez]

Created attachment 456479 [details]
WIP Patch

[Chris Dumez]

Created attachment 456481 [details]
WIP Patch

[Chris Dumez]

Created attachment 456483 [details]
WIP Patch

[Chris Dumez]

Created attachment 456484 [details]
WIP Patch

[Chris Dumez]

Created attachment 456485 [details]
WIP Patch

[Chris Dumez]

Created attachment 456486 [details]
WIP Patch

[Chris Dumez]

Created attachment 456487 [details]
WIP Patch

[Chris Dumez]

Created attachment 456488 [details]
WIP Patch

[Chris Dumez]

Created attachment 456489 [details]
WIP Patch

[Chris Dumez]

Created attachment 456490 [details]
WIP Patch

[Chris Dumez]

Created attachment 456491 [details]
WIP Patch

[Chris Dumez]

Created attachment 456493 [details]
WIP Patch

[Chris Dumez]

Created attachment 456494 [details]
WIP Patch

[Chris Dumez]

Created attachment 456495 [details]
WIP Patch

[Chris Dumez]

Created attachment 456496 [details]
WIP Patch

[Chris Dumez]

Created attachment 456497 [details]
WIP Patch

[Chris Dumez]

Created attachment 456498 [details]
WIP Patch

[Chris Dumez]

Created attachment 456506 [details]
WIP Patch

[Chris Dumez]

Created attachment 456509 [details]
WIP Patch

[Chris Dumez]

Created attachment 456512 [details]
WIP Patch

[Chris Dumez]

Created attachment 456514 [details]
WIP Patch

[Chris Dumez]

Created attachment 456516 [details]
WIP Patch

[Chris Dumez]

Created attachment 456517 [details]
WIP Patch

[Chris Dumez]

Created attachment 456519 [details]
WIP Patch

[Chris Dumez]

Created attachment 456522 [details]
WIP Patch

[Chris Dumez]

Created attachment 456523 [details]
WIP Patch

[Chris Dumez]

Created attachment 456524 [details]
WIP Patch

[Chris Dumez]

Created attachment 456526 [details]
WIP Patch

[Chris Dumez]

Created attachment 456527 [details]
WIP Patch

[Chris Dumez]

Created attachment 456528 [details]
Patch

[Chris Dumez]

Created attachment 456592 [details]
Patch

[Chris Dumez]

Created attachment 456606 [details]
Patch

[Chris Dumez]

Patch is ready for review.

[Joseph Pecoraro]

Neat!!

[Geoffrey Garen]

Comment on attachment 456606 [details]
Patch

r=me

[Chris Dumez]

Created attachment 456714 [details]
Patch

[Chris Dumez]

Comment on attachment 456714 [details]
Patch

Clearing flags on attachment: 456714

Committed r292408 (249271@trunk): <https://commits.webkit.org/249271@trunk>

[Chris Dumez]

All reviewed patches have been landed.  Closing bug.

[Radar WebKit Bug Importer]

<rdar://problem/91307597>