| Summary: | [Mac] PasteboardMac.mm build fails | ||||||||||||||
|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|
| Product: | WebKit | Reporter: | Kentaro Hara <haraken> | ||||||||||||
| Component: | Platform | Assignee: | Kentaro Hara <haraken> | ||||||||||||
| Status: | RESOLVED FIXED | ||||||||||||||
| Severity: | Normal | CC: | ap, darin, enrica, morrita, webkit.review.bot | ||||||||||||
| Priority: | P2 | ||||||||||||||
| Version: | 528+ (Nightly build) | ||||||||||||||
| Hardware: | Unspecified | ||||||||||||||
| OS: | Unspecified | ||||||||||||||
| Attachments: |
|
||||||||||||||
Created attachment 127090 [details]
Patch
Comment on attachment 127090 [details] Patch Clearing flags on attachment: 127090 Committed r107770: <http://trac.webkit.org/changeset/107770> All reviewed patches have been landed. Closing bug. Comment on attachment 127090 [details] Patch View in context: https://bugs.webkit.org/attachment.cgi?id=127090&action=review > Source/WebCore/platform/mac/PasteboardMac.mm:322 > + NSString *string = nil; This is not the right fix. The code for NSFilenamesPboardType is wrong, and that’s the code triggering the warning, and initializing this to nil just hides that bug. Comment on attachment 127090 [details] Patch View in context: https://bugs.webkit.org/attachment.cgi?id=127090&action=review >> Source/WebCore/platform/mac/PasteboardMac.mm:322 >> + NSString *string = nil; > > This is not the right fix. > > The code for NSFilenamesPboardType is wrong, and that’s the code triggering the warning, and initializing this to nil just hides that bug. I got it. Let me fix it. Reopening to attach new patch. Created attachment 127147 [details]
Patch
Created attachment 127148 [details]
Patch
Attachment 127147 [details] did not pass style-queue:
Failed to run "['Tools/Scripts/update-webkit']" exit_code: 9
Updating OpenSource
Index mismatch: 0dfd183742a71cb5de5dadc3ae177fc72b63a194 != 9cdcda984def14b8bf8a32b6da6784c8a6ef7b3a
rereading 8567f8d3c2539a28a496edaf1048483e973975c2
M LayoutTests/fast/forms/radio-nested-labels.html
M LayoutTests/ChangeLog
107798 = 3671b2d23de7ade4cb1d1e78a3f6f7673db6a6c9 already exists! Why are we refetching it?
at /usr/lib/git-core/git-svn line 5210
Died at Tools/Scripts/update-webkit line 164.
If any of these errors are false positives, please file a bug against check-webkit-style.
Attachment 127148 [details] did not pass style-queue:
Failed to run "['Tools/Scripts/update-webkit']" exit_code: 9
Updating OpenSource
Index mismatch: 0dfd183742a71cb5de5dadc3ae177fc72b63a194 != 9cdcda984def14b8bf8a32b6da6784c8a6ef7b3a
rereading 8567f8d3c2539a28a496edaf1048483e973975c2
M LayoutTests/fast/forms/radio-nested-labels.html
M LayoutTests/ChangeLog
107798 = 3671b2d23de7ade4cb1d1e78a3f6f7673db6a6c9 already exists! Why are we refetching it?
at /usr/lib/git-core/git-svn line 5210
Died at Tools/Scripts/update-webkit line 164.
If any of these errors are false positives, please file a bug against check-webkit-style.
(In reply to comment #5) > (From update of attachment 127090 [details]) > View in context: https://bugs.webkit.org/attachment.cgi?id=127090&action=review > > >> Source/WebCore/platform/mac/PasteboardMac.mm:322 > >> + NSString *string = nil; > > > > This is not the right fix. > > > > The code for NSFilenamesPboardType is wrong, and that’s the code triggering the warning, and initializing this to nil just hides that bug. > > I got it. Let me fix it. Why is this wrong? Darin, could you explain? I understand that the code was wrong when the nil initialization was missing, but I don't understand why this fix isn't correct. (In reply to comment #11) > Why is this wrong? Darin, could you explain? I understand that the code was wrong when the nil initialization was missing, but I don't understand why this fix isn't correct. Here’s the old code: for (size_t i = 0; i < pathnames.size(); i++) string = [string length] ? @"\n" + pathnames[i] : pathnames[i]; string = [string precomposedStringWithCanonicalMapping]; That code will produce a string containing the last pathname in the vector, possibly with a newline prepended. It will not produce a string with multiple pathnames in it. Comment on attachment 127148 [details] Patch View in context: https://bugs.webkit.org/attachment.cgi?id=127148&action=review > Source/WebCore/platform/mac/PasteboardMac.mm:345 > + string = [NSString stringWithFormat:@"%@\n%@", string, pathname]; It’s usually best to avoid stringWithFormat. I suggest we use StringBuilder for this and convert to NSString only at the end. (In reply to comment #12) > (In reply to comment #11) > > Why is this wrong? Darin, could you explain? I understand that the code was wrong when the nil initialization was missing, but I don't understand why this fix isn't correct. > > Here’s the old code: > > for (size_t i = 0; i < pathnames.size(); i++) > string = [string length] ? @"\n" + pathnames[i] : pathnames[i]; > string = [string precomposedStringWithCanonicalMapping]; > > That code will produce a string containing the last pathname in the vector, possibly with a newline prepended. It will not produce a string with multiple pathnames in it. You're absolutely right, I missed that. The correct code is: string = [string length] ? string + @"\n" + pathnames[i] : pathnames[i]; I think that adding the nil initialization is ok, as long as the other fix goes in. Comment on attachment 127148 [details] Patch View in context: https://bugs.webkit.org/attachment.cgi?id=127148&action=review >> Source/WebCore/platform/mac/PasteboardMac.mm:345 >> + string = [NSString stringWithFormat:@"%@\n%@", string, pathname]; > > It’s usually best to avoid stringWithFormat. I suggest we use StringBuilder for this and convert to NSString only at the end. I think it is enough to change the code in the original for loop to string = [string length] ? string + @"\n" + pathnames[i] : pathnames[i] and of course leave the initialization to nil. (In reply to comment #15) > string = [string length] ? string + @"\n" + pathnames[i] : pathnames[i] OK, but that still won’t work, and might not even compile, because we can’t just add two NSString * together using the + operator. We need test coverage so we can know this is working code. (In reply to comment #16) > (In reply to comment #15) > > string = [string length] ? string + @"\n" + pathnames[i] : pathnames[i] > > OK, but that still won’t work, and might not even compile, because we can’t just add two NSString * together using the + operator. > > We need test coverage so we can know this is working code. You're right it does not. This code is ok: for (size_t i = 0; i < pathnames.size(); i++) string = [string length] ? string + String("\n") + pathnames[i] : pathnames[i]; Kentaro, if you want I can take care of this, since I'm the one who introduced the bug :-) (In reply to comment #17) > Kentaro, if you want I can take care of this, since I'm the one who introduced the bug :-) Good morning:-) I'll upload a patch in minutes. Created attachment 127257 [details]
Patch
(In reply to comment #13) > It’s usually best to avoid stringWithFormat. I suggest we use StringBuilder for this and convert to NSString only at the end. Darin: Uploaded a patch using StringBuilder. Comment on attachment 127257 [details] Patch View in context: https://bugs.webkit.org/attachment.cgi?id=127257&action=review > Source/WebCore/platform/mac/PasteboardMac.mm:350 > + } I don't think you need all these changes. The only line you need to change in the one in the original for loop to the following: string = [string length] ? string + String("\n") + pathnames[i] : pathnames[i]; (In reply to comment #21) > I don't think you need all these changes. > The only line you need to change in the one in the original for loop to the following: > string = [string length] ? string + String("\n") + pathnames[i] : pathnames[i]; As Darin pointed out, using StringBuilder would be better for performance. In the above code, the conversion from String to NSString happens at every concatenation. (In reply to comment #22) > (In reply to comment #21) > > I don't think you need all these changes. > > The only line you need to change in the one in the original for loop to the following: > > string = [string length] ? string + String("\n") + pathnames[i] : pathnames[i]; > > As Darin pointed out, using StringBuilder would be better for performance. In the above code, the conversion from String to NSString happens at every concatenation. That is right. Then, since we only need to deal with the NSString at the very end I would suggest: String pathnamesString; for (size_t i = 0; i < pathnames.size(); i++) pathnamesString = !pathnamesString.isEmpty() ? pathnamesString + "\n" + pathnames[i] : pathnames[i]; string = [(NSString *)pathnamesString precomposedStringWithCanonicalMapping]; if (string != nil) return string; (In reply to comment #23) > That is right. Then, since we only need to deal with the NSString at the very end I would suggest: > > String pathnamesString; > for (size_t i = 0; i < pathnames.size(); i++) > pathnamesString = !pathnamesString.isEmpty() ? pathnamesString + "\n" + pathnames[i] : pathnames[i]; > string = [(NSString *)pathnamesString precomposedStringWithCanonicalMapping]; > if (string != nil) > return string; Using StringBuilder for String concatenation is also important for performance. (In reply to comment #24) > (In reply to comment #23) > > That is right. Then, since we only need to deal with the NSString at the very end I would suggest: > > > > String pathnamesString; > > for (size_t i = 0; i < pathnames.size(); i++) > > pathnamesString = !pathnamesString.isEmpty() ? pathnamesString + "\n" + pathnames[i] : pathnames[i]; > > string = [(NSString *)pathnamesString precomposedStringWithCanonicalMapping]; > > if (string != nil) > > return string; > > Using StringBuilder for String concatenation is also important for performance. Ok, then I'd prefer this: StringBuilder builder; for (size_t i = 0; i < pathnames.size(); i++) builder.append(i ? "\n" + pathnames[i] : pathnames[i]); string = [(NSString *)builder.toString() precomposedStringWithCanonicalMapping]; Created attachment 127274 [details]
Patch
Enrica: Fixed as you pointed out.
Comment on attachment 127274 [details]
Patch
Looks great! Thanks for adding the test too :-)
Comment on attachment 127274 [details] Patch Clearing flags on attachment: 127274 Committed r107886: <http://trac.webkit.org/changeset/107886> All reviewed patches have been landed. Closing bug. |
Although the bots have been working fine, PasteboardMac.mm build fails in our local Mac environments due to an uninitialized variable. /Users/haraken/WebKit/Source/WebCore/platform/mac/PasteboardMac.mm:322: warning: 'string' may be used uninitialized in this function