107227 2013-01-17 22:14:15 -0800 Fix WTF::copyLCharsFromUCharSource() to compile with -Wshorten-64-to-32 2013-01-18 15:49:36 -0800 1 1 1 Unclassified WebKit Web Template Framework 528+ (Nightly build) Unspecified Unspecified RESOLVED FIXED P2 Normal --- 107093 1 ddkilzer ddkilzer benjamin ojan.autocc webkit.review.bot oldest_to_newest 810233 0 ddkilzer 2013-01-17 22:14:15 -0800 <http://webkit.org/b/000000> Reviewed by NOBODY (OOPS!). Fixes the following build error: ASCIIFastPath.h:117:59: error: implicit conversion loses integer precision: 'unsigned long' to 'const unsigned int' [-Werror,-Wshorten-64-to-32] const unsigned endLength = length - ucharsPerLoop + 1; ~~~~~~~~~ ~~~~~~~~~~~~~~~~~~~~~~~^~~ 1 error generated. * wtf/text/ASCIIFastPath.h: (WTF::copyLCharsFromUCharSource): Change local variables from unsigned to size_t. --- 2 files changed, 20 insertions(+), 2 deletions(-) 810234 1 183374 ddkilzer 2013-01-17 22:14:16 -0800 Created attachment 183374 Patch 810241 2 183374 ddkilzer 2013-01-17 22:22:06 -0800 Comment on attachment 183374 Patch View in context: https://bugs.webkit.org/attachment.cgi?id=183374&action=review > Source/WTF/ChangeLog:4 > + Fix WTF::copyLCharsFromUCharSource() to compile with -Wshorten-64-to-32 > + <107227> Oops! Trying to make webkit-patch replace bug ID placeholders in my patches. My re.sub() fu is weak! 810244 3 183376 ddkilzer 2013-01-17 22:25:18 -0800 Created attachment 183376 Patch v2 810250 4 183376 benjamin 2013-01-17 22:30:14 -0800 Comment on attachment 183376 Patch v2 View in context: https://bugs.webkit.org/attachment.cgi?id=183376&action=review > Source/WTF/wtf/text/ASCIIFastPath.h:115 > const uintptr_t sourceLoadSize = 32; // Process 32 bytes (16 UChars) each iteration > - const unsigned ucharsPerLoop = sourceLoadSize / sizeof(UChar); > + const size_t ucharsPerLoop = sourceLoadSize / sizeof(UChar); You may want to change sourceLoadSize to size_t for the case where sizeof(size_t) < sizeof(void*) 810262 5 ddkilzer 2013-01-17 22:38:09 -0800 (In reply to comment #4) > (From update of attachment 183376 [details]) > View in context: https://bugs.webkit.org/attachment.cgi?id=183376&action=review > > > Source/WTF/wtf/text/ASCIIFastPath.h:115 > > const uintptr_t sourceLoadSize = 32; // Process 32 bytes (16 UChars) each iteration > > - const unsigned ucharsPerLoop = sourceLoadSize / sizeof(UChar); > > + const size_t ucharsPerLoop = sourceLoadSize / sizeof(UChar); > > You may want to change sourceLoadSize to size_t for the case where sizeof(size_t) < sizeof(void*) On what systems would the size of size_t be smaller than the size of a pointer? I can't think of any off the top of my head, but it's late and I haven't worked with any "interesting" systems recently. :) This code is also inside this macro: #if OS(DARWIN) && (CPU(X86) || CPU(X86_64)) So I doubt uintptr_t and size_t are going to change sizes in those configurations. Looking at the code, though, I guess it would be a good idea to use size_t just to match the other local variables. Any reason why uintptr_t was chosen here to begin with? 810272 6 benjamin 2013-01-17 22:58:38 -0800 > On what systems would the size of size_t be smaller than the size of a pointer? I can't think of any off the top of my head, but it's late and I haven't worked with any "interesting" systems recently. :) I can just be wrong, but I am thinking about mixed ABI like x32. Although, I guess it is the other way around then and sizeof(size_t) > sizeof(void*). > This code is also inside this macro: > > #if OS(DARWIN) && (CPU(X86) || CPU(X86_64)) > > So I doubt uintptr_t and size_t are going to change sizes in those configurations. Right! I missed that. Yep, that does not matter in this case. > Looking at the code, though, I guess it would be a good idea to use size_t just to match the other local variables. Any reason why uintptr_t was chosen here to begin with? I think it was just to have a type semantically correct to explain the "/ size(...)" later. 810619 7 ddkilzer 2013-01-18 08:14:27 -0800 (In reply to comment #6) > > Looking at the code, though, I guess it would be a good idea to use size_t just to match the other local variables. Any reason why uintptr_t was chosen here to begin with? > > I think it was just to have a type semantically correct to explain the "/ size(...)" later. On second thought, I think I'll leave it unchanged for now. It can always be fixed later if it's an issue. Thanks! 810708 8 ddkilzer 2013-01-18 09:27:33 -0800 Committed r140162: <http://trac.webkit.org/changeset/140162> 811115 9 183376 darin 2013-01-18 15:49:36 -0800 Comment on attachment 183376 Patch v2 View in context: https://bugs.webkit.org/attachment.cgi?id=183376&action=review >>> Source/WTF/wtf/text/ASCIIFastPath.h:115 >>> + const size_t ucharsPerLoop = sourceLoadSize / sizeof(UChar); >> >> You may want to change sourceLoadSize to size_t for the case where sizeof(size_t) < sizeof(void*) > > On what systems would the size of size_t be smaller than the size of a pointer? I can't think of any off the top of my head, but it's late and I haven't worked with any "interesting" systems recently. :) > > This code is also inside this macro: > > #if OS(DARWIN) && (CPU(X86) || CPU(X86_64)) > > So I doubt uintptr_t and size_t are going to change sizes in those configurations. > > Looking at the code, though, I guess it would be a good idea to use size_t just to match the other local variables. Any reason why uintptr_t was chosen here to begin with? Agreed that it should be size_t just for consistency. But not sure that it earns its keep as a named variable, really, since it’s just used in the line below. I think that comment would be clearer if it was on a line by itself and the “32” could just be in the line below; the comment is a great one for the ucharsPerLoop initialization. 183374 2013-01-17 22:14:16 -0800 2013-01-17 22:25:17 -0800 Patch bug-107227-20130117221115.patch text/plain 2046 ddkilzer U3VidmVyc2lvbiBSZXZpc2lvbjogMTM5OTgzCmRpZmYgLS1naXQgYS9Tb3VyY2UvV1RGL0NoYW5n ZUxvZyBiL1NvdXJjZS9XVEYvQ2hhbmdlTG9nCmluZGV4IGM2NWNkN2Q1N2E0NmRhMDE3N2JhMzRj N2M2YzE4ZDEwMmM1YTI1ODcuLmRiOTZhMzg2OTNiMjZmZmY2OWY5NzI2MjI5YWZiZDlhNDJmMzA0 YjUgMTAwNjQ0Ci0tLSBhL1NvdXJjZS9XVEYvQ2hhbmdlTG9nCisrKyBiL1NvdXJjZS9XVEYvQ2hh bmdlTG9nCkBAIC0xLDUgKzEsMjMgQEAKIDIwMTMtMDEtMTcgIERhdmlkIEtpbHplciAgPGRka2ls emVyQGFwcGxlLmNvbT4KIAorICAgICAgICBGaXggV1RGOjpjb3B5TENoYXJzRnJvbVVDaGFyU291 cmNlKCkgdG8gY29tcGlsZSB3aXRoIC1Xc2hvcnRlbi02NC10by0zMgorICAgICAgICA8ATEwNzIy Nz4KKworICAgICAgICBSZXZpZXdlZCBieSBOT0JPRFkgKE9PUFMhKS4KKworICAgICAgICBGaXhl cyB0aGUgZm9sbG93aW5nIGJ1aWxkIGVycm9yOgorCisgICAgICAgICAgICBBU0NJSUZhc3RQYXRo Lmg6MTE3OjU5OiBlcnJvcjogaW1wbGljaXQgY29udmVyc2lvbiBsb3NlcyBpbnRlZ2VyIHByZWNp c2lvbjogJ3Vuc2lnbmVkIGxvbmcnIHRvICdjb25zdCB1bnNpZ25lZCBpbnQnIFstV2Vycm9yLC1X c2hvcnRlbi02NC10by0zMl0KKyAgICAgICAgICAgICAgICAgICAgY29uc3QgdW5zaWduZWQgZW5k TGVuZ3RoID0gbGVuZ3RoIC0gdWNoYXJzUGVyTG9vcCArIDE7CisgICAgICAgICAgICAgICAgICAg ICAgICAgICAgICAgICAgIH5+fn5+fn5+fiAgIH5+fn5+fn5+fn5+fn5+fn5+fn5+fn5+Xn5+Cisg ICAgICAgICAgICAxIGVycm9yIGdlbmVyYXRlZC4KKworICAgICAgICAqIHd0Zi90ZXh0L0FTQ0lJ RmFzdFBhdGguaDoKKyAgICAgICAgKFdURjo6Y29weUxDaGFyc0Zyb21VQ2hhclNvdXJjZSk6IENo YW5nZSBsb2NhbCB2YXJpYWJsZXMgZnJvbQorICAgICAgICB1bnNpZ25lZCB0byBzaXplX3QuCisK KzIwMTMtMDEtMTcgIERhdmlkIEtpbHplciAgPGRka2lsemVyQGFwcGxlLmNvbT4KKwogICAgICAg ICBGaXggdnByaW50Zl9zdGRlcnJfY29tbW9uKCkgdG8gY29tcGlsZSB3aXRoIC1Xc2hvcnRlbi02 NC10by0zMgogICAgICAgICA8aHR0cDovL3dlYmtpdC5vcmcvYi8xMDcxMjY+CiAKZGlmZiAtLWdp dCBhL1NvdXJjZS9XVEYvd3RmL3RleHQvQVNDSUlGYXN0UGF0aC5oIGIvU291cmNlL1dURi93dGYv dGV4dC9BU0NJSUZhc3RQYXRoLmgKaW5kZXggMDIzM2VmYzZlNGI4YWEwNDVkYTExM2NkMGYxZGZm YjExZjM1YWNmMi4uNjliZWFiODMwNjM4MTM4NjMyMDU5NjgwOGEwNzBkMzgyZWRkYjJlNyAxMDA2 NDQKLS0tIGEvU291cmNlL1dURi93dGYvdGV4dC9BU0NJSUZhc3RQYXRoLmgKKysrIGIvU291cmNl L1dURi93dGYvdGV4dC9BU0NJSUZhc3RQYXRoLmgKQEAgLTExMiw5ICsxMTIsOSBAQCBpbmxpbmUg dm9pZCBjb3B5TENoYXJzRnJvbVVDaGFyU291cmNlKExDaGFyKiBkZXN0aW5hdGlvbiwgY29uc3Qg VUNoYXIqIHNvdXJjZSwgcwogICAgIH0KIAogICAgIGNvbnN0IHVpbnRwdHJfdCBzb3VyY2VMb2Fk U2l6ZSA9IDMyOyAvLyBQcm9jZXNzIDMyIGJ5dGVzICgxNiBVQ2hhcnMpIGVhY2ggaXRlcmF0aW9u Ci0gICAgY29uc3QgdW5zaWduZWQgdWNoYXJzUGVyTG9vcCA9IHNvdXJjZUxvYWRTaXplIC8gc2l6 ZW9mKFVDaGFyKTsKKyAgICBjb25zdCBzaXplX3QgdWNoYXJzUGVyTG9vcCA9IHNvdXJjZUxvYWRT aXplIC8gc2l6ZW9mKFVDaGFyKTsKICAgICBpZiAobGVuZ3RoID4gdWNoYXJzUGVyTG9vcCkgewot ICAgICAgICBjb25zdCB1bnNpZ25lZCBlbmRMZW5ndGggPSBsZW5ndGggLSB1Y2hhcnNQZXJMb29w ICsgMTsKKyAgICAgICAgY29uc3Qgc2l6ZV90IGVuZExlbmd0aCA9IGxlbmd0aCAtIHVjaGFyc1Bl ckxvb3AgKyAxOwogICAgICAgICBmb3IgKDsgaSA8IGVuZExlbmd0aDsgaSArPSB1Y2hhcnNQZXJM b29wKSB7CiAjaWZuZGVmIE5ERUJVRwogICAgICAgICAgICAgZm9yICh1bnNpZ25lZCBjaGVja0lu ZGV4ID0gMDsgY2hlY2tJbmRleCA8IHVjaGFyc1Blckxvb3A7ICsrY2hlY2tJbmRleCkK 183376 2013-01-17 22:25:18 -0800 2013-01-18 15:49:36 -0800 Patch v2 bug-107227-20130117222218.patch text/plain 2065 ddkilzer U3VidmVyc2lvbiBSZXZpc2lvbjogMTM5OTgzCmRpZmYgLS1naXQgYS9Tb3VyY2UvV1RGL0NoYW5n ZUxvZyBiL1NvdXJjZS9XVEYvQ2hhbmdlTG9nCmluZGV4IGM2NWNkN2Q1N2E0NmRhMDE3N2JhMzRj N2M2YzE4ZDEwMmM1YTI1ODcuLjJhNjkwOTg1YTUwOTQ0NGJkMzAxNmU1MjhmMWI3YmNhOGM1YmVk YTEgMTAwNjQ0Ci0tLSBhL1NvdXJjZS9XVEYvQ2hhbmdlTG9nCisrKyBiL1NvdXJjZS9XVEYvQ2hh bmdlTG9nCkBAIC0xLDUgKzEsMjMgQEAKIDIwMTMtMDEtMTcgIERhdmlkIEtpbHplciAgPGRka2ls emVyQGFwcGxlLmNvbT4KIAorICAgICAgICBGaXggV1RGOjpjb3B5TENoYXJzRnJvbVVDaGFyU291 cmNlKCkgdG8gY29tcGlsZSB3aXRoIC1Xc2hvcnRlbi02NC10by0zMgorICAgICAgICA8aHR0cDov L3dlYmtpdC5vcmcvYi8xMDcyMjc+CisKKyAgICAgICAgUmV2aWV3ZWQgYnkgTk9CT0RZIChPT1BT ISkuCisKKyAgICAgICAgRml4ZXMgdGhlIGZvbGxvd2luZyBidWlsZCBlcnJvcjoKKworICAgICAg ICAgICAgQVNDSUlGYXN0UGF0aC5oOjExNzo1OTogZXJyb3I6IGltcGxpY2l0IGNvbnZlcnNpb24g bG9zZXMgaW50ZWdlciBwcmVjaXNpb246ICd1bnNpZ25lZCBsb25nJyB0byAnY29uc3QgdW5zaWdu ZWQgaW50JyBbLVdlcnJvciwtV3Nob3J0ZW4tNjQtdG8tMzJdCisgICAgICAgICAgICAgICAgICAg IGNvbnN0IHVuc2lnbmVkIGVuZExlbmd0aCA9IGxlbmd0aCAtIHVjaGFyc1Blckxvb3AgKyAxOwor ICAgICAgICAgICAgICAgICAgICAgICAgICAgICAgICAgICB+fn5+fn5+fn4gICB+fn5+fn5+fn5+ fn5+fn5+fn5+fn5+fl5+fgorICAgICAgICAgICAgMSBlcnJvciBnZW5lcmF0ZWQuCisKKyAgICAg ICAgKiB3dGYvdGV4dC9BU0NJSUZhc3RQYXRoLmg6CisgICAgICAgIChXVEY6OmNvcHlMQ2hhcnNG cm9tVUNoYXJTb3VyY2UpOiBDaGFuZ2UgbG9jYWwgdmFyaWFibGVzIGZyb20KKyAgICAgICAgdW5z aWduZWQgdG8gc2l6ZV90LgorCisyMDEzLTAxLTE3ICBEYXZpZCBLaWx6ZXIgIDxkZGtpbHplckBh cHBsZS5jb20+CisKICAgICAgICAgRml4IHZwcmludGZfc3RkZXJyX2NvbW1vbigpIHRvIGNvbXBp bGUgd2l0aCAtV3Nob3J0ZW4tNjQtdG8tMzIKICAgICAgICAgPGh0dHA6Ly93ZWJraXQub3JnL2Iv MTA3MTI2PgogCmRpZmYgLS1naXQgYS9Tb3VyY2UvV1RGL3d0Zi90ZXh0L0FTQ0lJRmFzdFBhdGgu aCBiL1NvdXJjZS9XVEYvd3RmL3RleHQvQVNDSUlGYXN0UGF0aC5oCmluZGV4IDAyMzNlZmM2ZTRi OGFhMDQ1ZGExMTNjZDBmMWRmZmIxMWYzNWFjZjIuLjY5YmVhYjgzMDYzODEzODYzMjA1OTY4MDhh MDcwZDM4MmVkZGIyZTcgMTAwNjQ0Ci0tLSBhL1NvdXJjZS9XVEYvd3RmL3RleHQvQVNDSUlGYXN0 UGF0aC5oCisrKyBiL1NvdXJjZS9XVEYvd3RmL3RleHQvQVNDSUlGYXN0UGF0aC5oCkBAIC0xMTIs OSArMTEyLDkgQEAgaW5saW5lIHZvaWQgY29weUxDaGFyc0Zyb21VQ2hhclNvdXJjZShMQ2hhciog ZGVzdGluYXRpb24sIGNvbnN0IFVDaGFyKiBzb3VyY2UsIHMKICAgICB9CiAKICAgICBjb25zdCB1 aW50cHRyX3Qgc291cmNlTG9hZFNpemUgPSAzMjsgLy8gUHJvY2VzcyAzMiBieXRlcyAoMTYgVUNo YXJzKSBlYWNoIGl0ZXJhdGlvbgotICAgIGNvbnN0IHVuc2lnbmVkIHVjaGFyc1Blckxvb3AgPSBz b3VyY2VMb2FkU2l6ZSAvIHNpemVvZihVQ2hhcik7CisgICAgY29uc3Qgc2l6ZV90IHVjaGFyc1Bl ckxvb3AgPSBzb3VyY2VMb2FkU2l6ZSAvIHNpemVvZihVQ2hhcik7CiAgICAgaWYgKGxlbmd0aCA+ IHVjaGFyc1Blckxvb3ApIHsKLSAgICAgICAgY29uc3QgdW5zaWduZWQgZW5kTGVuZ3RoID0gbGVu Z3RoIC0gdWNoYXJzUGVyTG9vcCArIDE7CisgICAgICAgIGNvbnN0IHNpemVfdCBlbmRMZW5ndGgg PSBsZW5ndGggLSB1Y2hhcnNQZXJMb29wICsgMTsKICAgICAgICAgZm9yICg7IGkgPCBlbmRMZW5n dGg7IGkgKz0gdWNoYXJzUGVyTG9vcCkgewogI2lmbmRlZiBOREVCVUcKICAgICAgICAgICAgIGZv ciAodW5zaWduZWQgY2hlY2tJbmRleCA9IDA7IGNoZWNrSW5kZXggPCB1Y2hhcnNQZXJMb29wOyAr K2NoZWNrSW5kZXgpCg==