Bug 175988

Summary: Add String::format variant that takes va_args
Product: WebKit Reporter: Eric Carlson <eric.carlson>
Component: Web Template FrameworkAssignee: Eric Carlson <eric.carlson>
Status: RESOLVED FIXED    
Severity: Normal CC: benjamin, buildbot, cdumez, cmarcelo, commit-queue, darin, dbates, ggaren, jer.noble, webkit-bug-importer
Priority: P2 Keywords: InRadar
Version: Other   
Hardware: Unspecified   
OS: Unspecified   
See Also: https://bugs.webkit.org/show_bug.cgi?id=176035
Attachments:
Description Flags
Proposed patch.
none
Address post-review feedback. eric.carlson: commit-queue+

Description Eric Carlson 2017-08-25 10:32:58 PDT
Add String::format variant that takes va_args so it can be used from a variadic function.
Comment 1 Eric Carlson 2017-08-25 11:37:08 PDT
Created attachment 319091 [details]
Proposed patch.
Comment 2 Radar WebKit Bug Importer 2017-08-25 11:37:55 PDT
<rdar://problem/34085521>
Comment 3 WebKit Commit Bot 2017-08-25 13:24:01 PDT
Comment on attachment 319091 [details]
Proposed patch.

Clearing flags on attachment: 319091

Committed r221203: <http://trac.webkit.org/changeset/221203>
Comment 4 WebKit Commit Bot 2017-08-25 13:24:03 PDT
All reviewed patches have been landed.  Closing bug.
Comment 5 Geoffrey Garen 2017-08-25 15:23:04 PDT
Comment on attachment 319091 [details]
Proposed patch.

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

> Source/WTF/wtf/text/WTFString.cpp:471
> +    va_copy(argsCopy, args);

This function calls "va_end(args);" in two places. It also passes "args" to some callees. I think all uses of "args" need to become uses of "argsCopy", no?
Comment 6 Eric Carlson 2017-08-25 16:45:02 PDT
(In reply to Geoffrey Garen from comment #5)
> Comment on attachment 319091 [details]
> Proposed patch.
> 
> View in context:
> https://bugs.webkit.org/attachment.cgi?id=319091&action=review
> 
> > Source/WTF/wtf/text/WTFString.cpp:471
> > +    va_copy(argsCopy, args);
> 
> This function calls "va_end(args);" in two places. It also passes "args" to
> some callees. I think all uses of "args" need to become uses of "argsCopy",
> no?

Oops, "va_end(args)" is definitely wrong, but AFAIK the other uses of "args" is correct.
Comment 7 Darin Adler 2017-08-25 20:51:06 PDT
We should stop using String::format.
Comment 8 Darin Adler 2017-08-25 20:54:48 PDT
And instead use something that is type safe. Like use makeString.
Comment 9 Eric Carlson 2017-08-28 08:41:59 PDT
Created attachment 319179 [details]
Address post-review feedback.
Comment 10 Eric Carlson 2017-08-28 14:01:55 PDT
(In reply to Darin Adler from comment #8)
> And instead use something that is type safe. Like use makeString.

Filed bug 176035 for this suggestion.