123406 2013-10-28 08:54:49 -0700 check-webkit-style should support C++11 rvalue references 2014-03-21 05:58:36 -0700 1 1 1 Unclassified WebKit Tools / Tests 528+ (Nightly build) Unspecified Unspecified RESOLVED FIXED P2 Normal --- 1 svillar webkit-unassigned benjamin bfulgham cjerdonek commit-queue glenn gustavo llango.u-szeged ossy rniwa sam svillar zan oldest_to_newest 944101 0 svillar 2013-10-28 08:54:49 -0700 The script currently considers: T&& as a mistake due to the missing whitespace before the two ampersands. 952485 1 217405 llango.u-szeged 2013-11-20 01:11:46 -0800 Created attachment 217405 Patch 952531 2 217405 zan 2013-11-20 03:37:29 -0800 Comment on attachment 217405 Patch This looks OK, but a small test case should be included. 952534 3 llango.u-szeged 2013-11-20 03:45:03 -0800 (In reply to comment #2) > (From update of attachment 217405 [details]) > This looks OK, but a small test case should be included. Ok, i'm on it. 952538 4 217418 llango.u-szeged 2013-11-20 04:48:41 -0800 Created attachment 217418 Patch 952627 5 217418 bfulgham 2013-11-20 09:29:51 -0800 Comment on attachment 217418 Patch View in context: https://bugs.webkit.org/attachment.cgi?id=217418&action=review A good start, but I think your logic in the "check_style" change is flawed. I think you allow rvalue references now at the cost of ignoring boolean expressions crossing multiple lines. Please add a test for the multiple-line case and fix the logic for identifying rvalue references. > Tools/Scripts/webkitpy/style/checkers/cpp.py:2626 > + if cleansed_line.strip().endswith('||') or cleansed_line.strip().endswith(' &&'): Won't strip() remove all the whitespace? Will we ever match ' &&'? > Tools/Scripts/webkitpy/style/checkers/cpp_unittest.py:629 > + self.assert_lint('T&&', '') We also need a test that shows that we catch instances of boolean expressions spanning multiple lines. I believe that your change will break that style check. You can prove me wrong by including a test case! :-) 952950 6 llango.u-szeged 2013-11-20 23:05:32 -0800 (In reply to comment #5) > (From update of attachment 217418 [details]) > View in context: https://bugs.webkit.org/attachment.cgi?id=217418&action=review > > A good start, but I think your logic in the "check_style" change is flawed. I think you allow rvalue references now at the cost of ignoring boolean expressions crossing multiple lines. Please add a test for the multiple-line case and fix the logic for identifying rvalue references. > > > Tools/Scripts/webkitpy/style/checkers/cpp.py:2626 > > + if cleansed_line.strip().endswith('||') or cleansed_line.strip().endswith(' &&'): > > Won't strip() remove all the whitespace? Will we ever match ' &&'? > > > Tools/Scripts/webkitpy/style/checkers/cpp_unittest.py:629 > > + self.assert_lint('T&&', '') > > We also need a test that shows that we catch instances of boolean expressions spanning multiple lines. I believe that your change will break that style check. You can prove me wrong by including a test case! :-) stip() method only remove the leading end trailing whitespaces. It won't remove the whitespace before '&&'. Just to be sure a i checked it before i uploaded the patch. End already exists a test that check the multiline conditions (last assert in 'test_brace_at_begin_of_line'). 952951 7 217418 bfulgham 2013-11-20 23:19:48 -0800 Comment on attachment 217418 Patch View in context: https://bugs.webkit.org/attachment.cgi?id=217418&action=review R=me >>> Tools/Scripts/webkitpy/style/checkers/cpp.py:2626 >>> + if cleansed_line.strip().endswith('||') or cleansed_line.strip().endswith(' &&'): >> >> Won't strip() remove all the whitespace? Will we ever match ' &&'? > > stip() method only remove the leading end trailing whitespaces. It won't remove the whitespace before '&&'. Just to be sure a i checked it before i uploaded the patch. End already exists a test that check the multiline conditions (last assert in 'test_brace_at_begin_of_line'). Great! Thanks for confirming. 952956 8 217418 commit-queue 2013-11-20 23:43:49 -0800 Comment on attachment 217418 Patch Clearing flags on attachment: 217418 Committed r159612: <http://trac.webkit.org/changeset/159612> 952957 9 commit-queue 2013-11-20 23:43:51 -0800 All reviewed patches have been landed. Closing bug. 993039 10 llango.u-szeged 2014-03-21 05:58:36 -0700 *** Bug 73395 has been marked as a duplicate of this bug. *** 217405 2013-11-20 01:11:46 -0800 2013-11-20 04:48:35 -0800 Patch bug-123406-20131120011145.patch text/plain 1547 llango.u-szeged U3VidmVyc2lvbiBSZXZpc2lvbjogMTU5NDk5CmRpZmYgLS1naXQgYS9Ub29scy9DaGFuZ2VMb2cg Yi9Ub29scy9DaGFuZ2VMb2cKaW5kZXggYzJkZmJmMmZiOWViOWM5NmE2M2NhZjE1MWQzZDIzZjlh Y2NmNjU0OS4uODkwNzM0MjgzMDk5YTM4ZmZjZTQyMDBiZGE3MjAyZWY1ZWM0YzMwMCAxMDA2NDQK LS0tIGEvVG9vbHMvQ2hhbmdlTG9nCisrKyBiL1Rvb2xzL0NoYW5nZUxvZwpAQCAtMSwzICsxLDEz IEBACisyMDEzLTExLTIwICBMw6FzemzDsyBMYW5nw7MgIDxsYW5nb0BpbmYudS1zemVnZWQuaHU+ CisKKyAgICAgICAgY2hlY2std2Via2l0LXN0eWxlIHNob3VsZCBzdXBwb3J0IEMrKzExIHJ2YWx1 ZSByZWZlcmVuY2VzLgorICAgICAgICBodHRwczovL2J1Z3Mud2Via2l0Lm9yZy9zaG93X2J1Zy5j Z2k/aWQ9MTIzNDA2CisKKyAgICAgICAgUmV2aWV3ZWQgYnkgTk9CT0RZIChPT1BTISkuCisKKyAg ICAgICAgKiBTY3JpcHRzL3dlYmtpdHB5L3N0eWxlL2NoZWNrZXJzL2NwcC5weToKKyAgICAgICAg KGNoZWNrX3N0eWxlKToKKwogMjAxMy0xMS0xOSAgUnlvc3VrZSBOaXdhICA8cm5pd2FAd2Via2l0 Lm9yZz4KIAogICAgICAgICBZZXQgYW5vdGhlciBidWlsZCBmaXguIEp1c3QgYWxsb3cgYW55IGNo YXJhY3Rlci4KZGlmZiAtLWdpdCBhL1Rvb2xzL1NjcmlwdHMvd2Via2l0cHkvc3R5bGUvY2hlY2tl cnMvY3BwLnB5IGIvVG9vbHMvU2NyaXB0cy93ZWJraXRweS9zdHlsZS9jaGVja2Vycy9jcHAucHkK aW5kZXggNWUwZmQwMTJhOWJiZmNmZDUwYmFlOGUwN2QyNTg1YzdiODE0MmFkMC4uMmQ1NTliZDcz MzY5ZDg1YjI2YzE5YTk3NWY0MWRkMmZhNWFhOTFhNyAxMDA2NDQKLS0tIGEvVG9vbHMvU2NyaXB0 cy93ZWJraXRweS9zdHlsZS9jaGVja2Vycy9jcHAucHkKKysrIGIvVG9vbHMvU2NyaXB0cy93ZWJr aXRweS9zdHlsZS9jaGVja2Vycy9jcHAucHkKQEAgLTI2MjMsNyArMjYyMyw3IEBAIGRlZiBjaGVj a19zdHlsZShjbGVhbl9saW5lcywgbGluZV9udW1iZXIsIGZpbGVfZXh0ZW5zaW9uLCBjbGFzc19z dGF0ZSwgZmlsZV9zdGF0CiAgICAgICAgIGVycm9yKGxpbmVfbnVtYmVyLCAnd2hpdGVzcGFjZS9u ZXdsaW5lJywgNCwKICAgICAgICAgICAgICAgJ01vcmUgdGhhbiBvbmUgY29tbWFuZCBvbiB0aGUg c2FtZSBsaW5lJykKIAotICAgIGlmIGNsZWFuc2VkX2xpbmUuc3RyaXAoKS5lbmRzd2l0aCgnfHwn KSBvciBjbGVhbnNlZF9saW5lLnN0cmlwKCkuZW5kc3dpdGgoJyYmJyk6CisgICAgaWYgY2xlYW5z ZWRfbGluZS5zdHJpcCgpLmVuZHN3aXRoKCd8fCcpIG9yIGNsZWFuc2VkX2xpbmUuc3RyaXAoKS5l bmRzd2l0aCgnICYmJyk6CiAgICAgICAgIGVycm9yKGxpbmVfbnVtYmVyLCAnd2hpdGVzcGFjZS9v cGVyYXRvcnMnLCA0LAogICAgICAgICAgICAgICAnQm9vbGVhbiBleHByZXNzaW9ucyB0aGF0IHNw YW4gbXVsdGlwbGUgbGluZXMgc2hvdWxkIGhhdmUgdGhlaXIgJwogICAgICAgICAgICAgICAnb3Bl cmF0b3JzIG9uIHRoZSBsZWZ0IHNpZGUgb2YgdGhlIGxpbmUgaW5zdGVhZCBvZiB0aGUgcmlnaHQg c2lkZS4nKQo= 217418 2013-11-20 04:48:41 -0800 2013-11-20 23:43:49 -0800 Patch bug-123406-20131120044841.patch text/plain 2355 llango.u-szeged U3VidmVyc2lvbiBSZXZpc2lvbjogMTU5NDk5CmRpZmYgLS1naXQgYS9Ub29scy9DaGFuZ2VMb2cg Yi9Ub29scy9DaGFuZ2VMb2cKaW5kZXggYzJkZmJmMmZiOWViOWM5NmE2M2NhZjE1MWQzZDIzZjlh Y2NmNjU0OS4uNDg0MTVlMjdiZmE0NWY5ZTUwOGFhYmMwMDU4OThlNWEzZTFjNjJjMSAxMDA2NDQK LS0tIGEvVG9vbHMvQ2hhbmdlTG9nCisrKyBiL1Rvb2xzL0NoYW5nZUxvZwpAQCAtMSwzICsxLDE2 IEBACisyMDEzLTExLTIwICBMw6FzemzDsyBMYW5nw7MgIDxsYW5nb0BpbmYudS1zemVnZWQuaHU+ CisKKyAgICAgICAgY2hlY2std2Via2l0LXN0eWxlIHNob3VsZCBzdXBwb3J0IEMrKzExIHJ2YWx1 ZSByZWZlcmVuY2VzLgorICAgICAgICBodHRwczovL2J1Z3Mud2Via2l0Lm9yZy9zaG93X2J1Zy5j Z2k/aWQ9MTIzNDA2CisKKyAgICAgICAgUmV2aWV3ZWQgYnkgTk9CT0RZIChPT1BTISkuCisKKyAg ICAgICAgKiBTY3JpcHRzL3dlYmtpdHB5L3N0eWxlL2NoZWNrZXJzL2NwcC5weToKKyAgICAgICAg KGNoZWNrX3N0eWxlKToKKyAgICAgICAgKiBTY3JpcHRzL3dlYmtpdHB5L3N0eWxlL2NoZWNrZXJz L2NwcF91bml0dGVzdC5weToKKyAgICAgICAgKENwcDExU3R5bGVUZXN0KToKKyAgICAgICAgKENw cDExU3R5bGVUZXN0LnRlc3RfcnZhdWxlX3JlZmVyZW5jZV9hdF9lbmRfb2ZfbGluZSk6CisKIDIw MTMtMTEtMTkgIFJ5b3N1a2UgTml3YSAgPHJuaXdhQHdlYmtpdC5vcmc+CiAKICAgICAgICAgWWV0 IGFub3RoZXIgYnVpbGQgZml4LiBKdXN0IGFsbG93IGFueSBjaGFyYWN0ZXIuCmRpZmYgLS1naXQg YS9Ub29scy9TY3JpcHRzL3dlYmtpdHB5L3N0eWxlL2NoZWNrZXJzL2NwcC5weSBiL1Rvb2xzL1Nj cmlwdHMvd2Via2l0cHkvc3R5bGUvY2hlY2tlcnMvY3BwLnB5CmluZGV4IDVlMGZkMDEyYTliYmZj ZmQ1MGJhZThlMDdkMjU4NWM3YjgxNDJhZDAuLjJkNTU5YmQ3MzM2OWQ4NWIyNmMxOWE5NzVmNDFk ZDJmYTVhYTkxYTcgMTAwNjQ0Ci0tLSBhL1Rvb2xzL1NjcmlwdHMvd2Via2l0cHkvc3R5bGUvY2hl Y2tlcnMvY3BwLnB5CisrKyBiL1Rvb2xzL1NjcmlwdHMvd2Via2l0cHkvc3R5bGUvY2hlY2tlcnMv Y3BwLnB5CkBAIC0yNjIzLDcgKzI2MjMsNyBAQCBkZWYgY2hlY2tfc3R5bGUoY2xlYW5fbGluZXMs IGxpbmVfbnVtYmVyLCBmaWxlX2V4dGVuc2lvbiwgY2xhc3Nfc3RhdGUsIGZpbGVfc3RhdAogICAg ICAgICBlcnJvcihsaW5lX251bWJlciwgJ3doaXRlc3BhY2UvbmV3bGluZScsIDQsCiAgICAgICAg ICAgICAgICdNb3JlIHRoYW4gb25lIGNvbW1hbmQgb24gdGhlIHNhbWUgbGluZScpCiAKLSAgICBp ZiBjbGVhbnNlZF9saW5lLnN0cmlwKCkuZW5kc3dpdGgoJ3x8Jykgb3IgY2xlYW5zZWRfbGluZS5z dHJpcCgpLmVuZHN3aXRoKCcmJicpOgorICAgIGlmIGNsZWFuc2VkX2xpbmUuc3RyaXAoKS5lbmRz d2l0aCgnfHwnKSBvciBjbGVhbnNlZF9saW5lLnN0cmlwKCkuZW5kc3dpdGgoJyAmJicpOgogICAg ICAgICBlcnJvcihsaW5lX251bWJlciwgJ3doaXRlc3BhY2Uvb3BlcmF0b3JzJywgNCwKICAgICAg ICAgICAgICAgJ0Jvb2xlYW4gZXhwcmVzc2lvbnMgdGhhdCBzcGFuIG11bHRpcGxlIGxpbmVzIHNo b3VsZCBoYXZlIHRoZWlyICcKICAgICAgICAgICAgICAgJ29wZXJhdG9ycyBvbiB0aGUgbGVmdCBz aWRlIG9mIHRoZSBsaW5lIGluc3RlYWQgb2YgdGhlIHJpZ2h0IHNpZGUuJykKZGlmZiAtLWdpdCBh L1Rvb2xzL1NjcmlwdHMvd2Via2l0cHkvc3R5bGUvY2hlY2tlcnMvY3BwX3VuaXR0ZXN0LnB5IGIv VG9vbHMvU2NyaXB0cy93ZWJraXRweS9zdHlsZS9jaGVja2Vycy9jcHBfdW5pdHRlc3QucHkKaW5k ZXggMDc5YTQ2OTkwZjk2MzU3YTJmNWJlYTQxNmIxYTdkY2Q3MjI1MDUwMC4uZTE0MzViYjc0MjU4 MjU4MGU1YWQzM2E3NjQxM2VlNmY2ODAwMTEzNyAxMDA2NDQKLS0tIGEvVG9vbHMvU2NyaXB0cy93 ZWJraXRweS9zdHlsZS9jaGVja2Vycy9jcHBfdW5pdHRlc3QucHkKKysrIGIvVG9vbHMvU2NyaXB0 cy93ZWJraXRweS9zdHlsZS9jaGVja2Vycy9jcHBfdW5pdHRlc3QucHkKQEAgLTYyNCw2ICs2MjQs MTEgQEAgY2xhc3MgRnVuY3Rpb25EZXRlY3Rpb25UZXN0KENwcFN0eWxlVGVzdEJhc2UpOgogICAg ICAgICAgICAgZGV0ZWN0aW9uX2xpbmU9MikKIAogCitjbGFzcyBDcHAxMVN0eWxlVGVzdChDcHBT dHlsZVRlc3RCYXNlKToKKyAgICBkZWYgdGVzdF9ydmF1bGVfcmVmZXJlbmNlX2F0X2VuZF9vZl9s aW5lKHNlbGYpOgorICAgICAgICBzZWxmLmFzc2VydF9saW50KCdUJiYnLCAnJykKKworCiBjbGFz cyBDcHBTdHlsZVRlc3QoQ3BwU3R5bGVUZXN0QmFzZSk6CiAKICAgICBkZWYgdGVzdF9hc21fbGlu ZXNfaWdub3JlZChzZWxmKToK