Bug 108103 - String constructed from Literals should be non-empty
Summary: String constructed from Literals should be non-empty
Status: RESOLVED FIXED
Alias: None
Product: WebKit
Classification: Unclassified
Component: Web Template Framework (show other bugs)
Version: 528+ (Nightly build)
Hardware: Unspecified Unspecified
: P2 Normal
Assignee: Benjamin Poulain
URL:
Keywords:
Depends on:
Blocks:
 
Reported: 2013-01-28 12:41 PST by Benjamin Poulain
Modified: 2013-01-29 10:37 PST (History)
6 users (show)

See Also:


Attachments
Patch (3.45 KB, patch)
2013-01-28 12:47 PST, Benjamin Poulain
no flags Details | Formatted Diff | Diff

Note You need to log in before you can comment on or make changes to this bug.
Description Benjamin Poulain 2013-01-28 12:41:40 PST
Empty strings can be evil and should be marked as such.
Comment 1 Benjamin Poulain 2013-01-28 12:47:46 PST
Created attachment 185038 [details]
Patch
Comment 2 Benjamin Poulain 2013-01-28 17:30:34 PST
Comment on attachment 185038 [details]
Patch

Clearing flags on attachment: 185038

Committed r141030: <http://trac.webkit.org/changeset/141030>
Comment 3 Benjamin Poulain 2013-01-28 17:30:36 PST
All reviewed patches have been landed.  Closing bug.
Comment 4 Darin Adler 2013-01-29 09:50:46 PST
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().
Comment 5 Benjamin Poulain 2013-01-29 10:16:00 PST
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.
Comment 6 Darin Adler 2013-01-29 10:37:51 PST
No, probably not really confusing in practice.