Chromium Code Reviews
|
|
Chromium Code Reviews
Issue
2608423003:
Update methods requiring Get prefix to avoid collisions with Blink rewrite. (Closed)
DescriptionUpdate methods requiring Get prefix to avoid collisions with Blink rewrite.
This CL adds a few more methods that cause name collisions when Blink
is rewritten to match Chromium style. In addition, it removes the hash
method from the blacklist.
BUG=582312, 677166
Committed: https://crrev.com/0020d5c9503dd59d2caf28999aa38bc600206303
Cr-Commit-Position: refs/heads/master@{#441429}
Patch Set 1 #
Total comments: 4
Patch Set 2 : Add more method names. #
Total comments: 2
Patch Set 3 : Fix comment. #
Messages
Total messages: 25 (13 generated)
The CQ bit was checked by nasko@chromium.org to run a CQ dry run
Dry run: CQ is trying da patch. Follow status at https://chromium-cq-status.appspot.com/v2/patch-status/codereview.chromium.or...
nasko@chromium.org changed reviewers: + lukasza@chromium.org
Hey Lukasz, Can you review this CL for me? It adds a few more methods to the list requiring Get prefix and removes hash from the blacklist. Thanks in advance! Nasko
The CQ bit was unchecked by commit-bot@chromium.org
Dry run: This issue passed the CQ dry run.
https://codereview.chromium.org/2608423003/diff/1/tools/clang/rewrite_to_chro... File tools/clang/rewrite_to_chrome_style/RewriteToChromeStyle.cpp (left): https://codereview.chromium.org/2608423003/diff/1/tools/clang/rewrite_to_chro... tools/clang/rewrite_to_chrome_style/RewriteToChromeStyle.cpp:387: "layoutSize", We need to add "hash" here, to force renaming into "GetHash", right? https://codereview.chromium.org/2608423003/diff/1/tools/clang/rewrite_to_chro... File tools/clang/rewrite_to_chrome_style/RewriteToChromeStyle.cpp (right): https://codereview.chromium.org/2608423003/diff/1/tools/clang/rewrite_to_chro... tools/clang/rewrite_to_chrome_style/RewriteToChromeStyle.cpp:420: "wordBoundaries", Can you please also add method names from https://crbug.com/582312#c93 : - frontend - heapObjectHeader - midpointState - resource - vector - wrapperTypeInfo
https://codereview.chromium.org/2608423003/diff/1/tools/clang/rewrite_to_chro... File tools/clang/rewrite_to_chrome_style/RewriteToChromeStyle.cpp (left): https://codereview.chromium.org/2608423003/diff/1/tools/clang/rewrite_to_chro... tools/clang/rewrite_to_chrome_style/RewriteToChromeStyle.cpp:387: "layoutSize", On 2017/01/04 17:12:46, Łukasz Anforowicz wrote: > We need to add "hash" here, to force renaming into "GetHash", right? Done. https://codereview.chromium.org/2608423003/diff/1/tools/clang/rewrite_to_chro... File tools/clang/rewrite_to_chrome_style/RewriteToChromeStyle.cpp (right): https://codereview.chromium.org/2608423003/diff/1/tools/clang/rewrite_to_chro... tools/clang/rewrite_to_chrome_style/RewriteToChromeStyle.cpp:420: "wordBoundaries", On 2017/01/04 17:12:46, Łukasz Anforowicz wrote: > Can you please also add method names from https://crbug.com/582312#c93 : > > - frontend > - heapObjectHeader > - midpointState > - resource > - vector > - wrapperTypeInfo Done.
Thanks - LGTM.
danakj@chromium.org changed reviewers: + danakj@chromium.org
LGTM
Oh is it that these are only for methods, and hash needs to handle static too?
On 2017/01/04 18:20:23, danakj_OOO_and_slow wrote: > Oh is it that these are only for methods, and hash needs to handle static too? shouldPrefixFunctionName matcher and the ShouldPrefixFunctionName predicate are consulted for all of 1) free functions, 2) static methods, 3) instance methods. That is: A) GetNameForDecl/clang::FunctionDecl overload is invoked for all 3 cases and B) GuessNameForUnresolvedDependentNode is applicable only to static methods and instance methods (but doesn't distinguish between these 2 cases). Also - dcheng@ had a question if it is OK to move the hash to only be blacklisted for instance methods. I think after this CL: i) there will be no blacklisting of "hash" - the blacklisting used to be in IsBlacklistedFunctionName (which is applicable to all of 1) free functions, 2) static methods, 3) instance methods). ii) "hash" should get a "get" prefix in all 3 cases - see the answer to the question from danakj@ above. So - I think this CL is still LGTM.
The CQ bit was checked by nasko@chromium.org to run a CQ dry run
Dry run: CQ is trying da patch. Follow status at https://chromium-cq-status.appspot.com/v2/patch-status/codereview.chromium.or...
https://codereview.chromium.org/2608423003/diff/20001/tools/clang/rewrite_to_... File tools/clang/rewrite_to_chrome_style/RewriteToChromeStyle.cpp (right): https://codereview.chromium.org/2608423003/diff/20001/tools/clang/rewrite_to_... tools/clang/rewrite_to_chrome_style/RewriteToChromeStyle.cpp:359: // Methods that are named similarily to a type - they should be prefixed Can you fix this comment - "Method" is incorrect here as this is more widely applicable as per Lucasz' comment. Then LGTM
The CQ bit was checked by nasko@chromium.org
The patchset sent to the CQ was uploaded after l-g-t-m from danakj@chromium.org, lukasza@chromium.org Link to the patchset: https://codereview.chromium.org/2608423003/#ps40001 (title: "Fix comment.")
CQ is trying da patch. Follow status at https://chromium-cq-status.appspot.com/v2/patch-status/codereview.chromium.or...
https://codereview.chromium.org/2608423003/diff/20001/tools/clang/rewrite_to_... File tools/clang/rewrite_to_chrome_style/RewriteToChromeStyle.cpp (right): https://codereview.chromium.org/2608423003/diff/20001/tools/clang/rewrite_to_... tools/clang/rewrite_to_chrome_style/RewriteToChromeStyle.cpp:359: // Methods that are named similarily to a type - they should be prefixed On 2017/01/04 18:44:51, danakj_OOO_and_slow wrote: > Can you fix this comment - "Method" is incorrect here as this is more widely > applicable as per Lucasz' comment. Then LGTM Done.
CQ is committing da patch.
Bot data: {"patchset_id": 40001, "attempt_start_ts": 1483556557545790,
"parent_rev": "29c0f0b3a10cb1444bd6d95514450cb11982c0b4", "commit_rev":
"b1e5749bb2363dc5b83505671cb4c2e0946cf819"}
Message was sent while issue was closed.
Description was changed from ========== Update methods requiring Get prefix to avoid collisions with Blink rewrite. This CL adds a few more methods that cause name collisions when Blink is rewritten to match Chromium style. In addition, it removes the hash method from the blacklist. BUG=582312, 677166 ========== to ========== Update methods requiring Get prefix to avoid collisions with Blink rewrite. This CL adds a few more methods that cause name collisions when Blink is rewritten to match Chromium style. In addition, it removes the hash method from the blacklist. BUG=582312, 677166 Review-Url: https://codereview.chromium.org/2608423003 ==========
Message was sent while issue was closed.
Committed patchset #3 (id:40001)
Message was sent while issue was closed.
Description was changed from ========== Update methods requiring Get prefix to avoid collisions with Blink rewrite. This CL adds a few more methods that cause name collisions when Blink is rewritten to match Chromium style. In addition, it removes the hash method from the blacklist. BUG=582312, 677166 Review-Url: https://codereview.chromium.org/2608423003 ========== to ========== Update methods requiring Get prefix to avoid collisions with Blink rewrite. This CL adds a few more methods that cause name collisions when Blink is rewritten to match Chromium style. In addition, it removes the hash method from the blacklist. BUG=582312, 677166 Committed: https://crrev.com/0020d5c9503dd59d2caf28999aa38bc600206303 Cr-Commit-Position: refs/heads/master@{#441429} ==========
Message was sent while issue was closed.
Patchset 3 (id:??) landed as https://crrev.com/0020d5c9503dd59d2caf28999aa38bc600206303 Cr-Commit-Position: refs/heads/master@{#441429} |
|||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||
