Bug 39494 – Change filenameFromString to return CString

• Status: RESOLVED
• Resolution: FIXED
• Component: Platform

[Kwang Yul Seo]

filenameFromString returns a newly allocated string and the caller must free the string. GTK and EFL ports use g_free while all others ports use fastFree. This is confusing because the same function behaves differently with respect to ports. Change filenameFromString to return CString.

[Kwang Yul Seo]

Created attachment 56720 [details]
Patch

[WebKit Review Bot]

Attachment 56720 [details] did not pass style-queue:

Failed to run "['WebKitTools/Scripts/check-webkit-style', '--no-squash']" exit_code: 1
WebCore/platform/gtk/FileSystemGtk.cpp:102:  Tests for true/false, null/non-null, and zero/non-zero should all be done without equality comparisons.  [readability/comparison_to_zero] [5]
WebCore/platform/gtk/FileSystemGtk.cpp:113:  Tests for true/false, null/non-null, and zero/non-zero should all be done without equality comparisons.  [readability/comparison_to_zero] [5]
WebCore/platform/gtk/FileSystemGtk.cpp:208:  Use 0 instead of NULL.  [readability/null] [5]
Total errors found: 3 in 9 files


If any of these errors are false positives, please file a bug against check-webkit-style.

[Darin Adler]

Comment on attachment 56720 [details]
Patch

Good change!

> -char* filenameFromString(const String&);
> +WTF::CString filenameFromString(const String&);

Just CString should be fine, no need for the "WTF::" prefix here.

I can't tell whether filenameFromString is allowed to return the null string or not. If it is allowed to return null, it seems that many call sites are missing code to handle this case and will crash. If it is not allowed to return null, then some call sites have extra unneeded code.

[Eric Seidel (no email)]

Comment on attachment 56720 [details]
Patch

r- per darin's comment above.

[Kwang Yul Seo]

Created attachment 56844 [details]
Revised patch

Remove "WTF:: prefix".

I will file a separate bug for null checks.

[WebKit Review Bot]

Attachment 56844 [details] did not pass style-queue:

Failed to run "['WebKitTools/Scripts/check-webkit-style', '--no-squash']" exit_code: 1
WebCore/platform/gtk/FileSystemGtk.cpp:102:  Tests for true/false, null/non-null, and zero/non-zero should all be done without equality comparisons.  [readability/comparison_to_zero] [5]
WebCore/platform/gtk/FileSystemGtk.cpp:113:  Tests for true/false, null/non-null, and zero/non-zero should all be done without equality comparisons.  [readability/comparison_to_zero] [5]
WebCore/platform/gtk/FileSystemGtk.cpp:208:  Use 0 instead of NULL.  [readability/null] [5]
Total errors found: 3 in 9 files


If any of these errors are false positives, please file a bug against check-webkit-style.

[Kwang Yul Seo]

(In reply to comment #3)
> Just CString should be fine, no need for the "WTF::" prefix here.

I tried to be consistent as two other functions use "WTF::" prefix here:

WTF::CString fileSystemRepresentation(const String&);
WTF::CString openTemporaryFile(const char* prefix, PlatformFileHandle&);

[Kent Tamura]

Comment on attachment 56844 [details]
Revised patch

> I tried to be consistent as two other functions use "WTF::" prefix here:

That's reasonable.
Style errors came from the existing code. So, this patch looks OK.

[WebKit Commit Bot]

Comment on attachment 56844 [details]
Revised patch

Clearing flags on attachment: 56844

Committed r60668: <http://trac.webkit.org/changeset/60668>

[WebKit Commit Bot]

All reviewed patches have been landed.  Closing bug.