Bug 108103 – String constructed from Literals should be non-empty

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

[Benjamin Poulain]

Empty strings can be evil and should be marked as such.

[Benjamin Poulain]

Created attachment 185038 [details]
Patch

[Benjamin Poulain]

Comment on attachment 185038 [details]
Patch

Clearing flags on attachment: 185038

Committed r141030: <http://trac.webkit.org/changeset/141030>

[Benjamin Poulain]

All reviewed patches have been landed.  Closing bug.

[Darin Adler]

Comment on attachment 185038 [details]
Patch

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

This patch disagrees with itself.

> Source/WTF/wtf/text/StringImpl.cpp:154
> +    ASSERT_WITH_MESSAGE(length, "Use StringImpl::empty() to create an empty string");

This says you should use StringImpl::empty().T

> Source/WebCore/html/HTMLMediaElement.cpp:691
> +            canPlay = emptyString();

This uses emptyString().

[Benjamin Poulain]

My rationale:

> > Source/WTF/wtf/text/StringImpl.cpp:154
> > +    ASSERT_WITH_MESSAGE(length, "Use StringImpl::empty() to create an empty string");
> 
> This says you should use StringImpl::empty().T

The assertion is in StringImpl. You can get here from multiple path. From StringImpl, StringImpl::empty() is the empty String.

> > Source/WebCore/html/HTMLMediaElement.cpp:691
> > +            canPlay = emptyString();
> 
> This uses emptyString().

The variable "canPlay" is a WTFString, so here I used the empty string constructor for WTFString.

If you think that can cause confusion, I can add similar assertions in WTFString, AtomicString and maybe JSString.

[Darin Adler]

No, probably not really confusing in practice.