Chromium Code Reviews
|
|
Chromium Code Reviews
Issue
1783923003:
rewrite_to_chrome_style: Blacklisted methods are not blink methods! (Closed)
Descriptionrewrite_to_chrome_style: Remove blacklisted methods with matcher
Consider blacklisted methods when looking at deciding to match against
them or not rather than deciding to not rename them much later.
R=dcheng
BUG=593446
Committed: https://crrev.com/27f1728f9c2cdfb9995c671783edd55bd66470be
Cr-Commit-Position: refs/heads/master@{#380735}
Patch Set 1 #
Total comments: 3
Patch Set 2 : rewrite-blacklist-namespace: blacklistfunction #
Total comments: 2
Patch Set 3 : rewrite-blacklist-namespace: local #Patch Set 4 : rewrite-blacklist-namespace: rewrite #Messages
Total messages: 19 (7 generated)
Depends on https://codereview.chromium.org/1785563002/
https://codereview.chromium.org/1783923003/diff/1/tools/clang/rewrite_to_chro... File tools/clang/rewrite_to_chrome_style/RewriteToChromeStyle.cpp (right): https://codereview.chromium.org/1783923003/diff/1/tools/clang/rewrite_to_chro... tools/clang/rewrite_to_chrome_style/RewriteToChromeStyle.cpp:138: // Blacklisted methods are not considered to be in blink so that they will 1) Would it make sense to just combine the iterator blacklist and this and have it in the same place? 2) Would it make sense to express this in the matcher instead? unless(anyOf(<really long list of current exceptions>, hasName("begin"), hasName("end"), hasName("rbegin"), hasName("rend"), ...)) so on and so forth.
https://codereview.chromium.org/1783923003/diff/1/tools/clang/rewrite_to_chro... File tools/clang/rewrite_to_chrome_style/RewriteToChromeStyle.cpp (right): https://codereview.chromium.org/1783923003/diff/1/tools/clang/rewrite_to_chro... tools/clang/rewrite_to_chrome_style/RewriteToChromeStyle.cpp:138: // Blacklisted methods are not considered to be in blink so that they will On 2016/03/10 22:52:46, dcheng wrote: > 1) Would it make sense to just combine the iterator blacklist and this and have > it in the same place? > > 2) Would it make sense to express this in the matcher instead? > > unless(anyOf(<really long list of current exceptions>, > hasName("begin"), > hasName("end"), > hasName("rbegin"), > hasName("rend"), > ...)) > so on and so forth. We made the iterator blacklist only kick in based on the return type being an iterator name though. That doesn't get expressed as easily with matchers (or IDK how atm). We could do that up here instead, but it'll be a separate list of methods still. Prefer that?
https://codereview.chromium.org/1783923003/diff/1/tools/clang/rewrite_to_chro... File tools/clang/rewrite_to_chrome_style/RewriteToChromeStyle.cpp (right): https://codereview.chromium.org/1783923003/diff/1/tools/clang/rewrite_to_chro... tools/clang/rewrite_to_chrome_style/RewriteToChromeStyle.cpp:138: // Blacklisted methods are not considered to be in blink so that they will On 2016/03/10 at 22:55:14, danakj wrote: > On 2016/03/10 22:52:46, dcheng wrote: > > 1) Would it make sense to just combine the iterator blacklist and this and have > > it in the same place? > > > > 2) Would it make sense to express this in the matcher instead? > > > > unless(anyOf(<really long list of current exceptions>, > > hasName("begin"), > > hasName("end"), > > hasName("rbegin"), > > hasName("rend"), > > ...)) > > so on and so forth. > > We made the iterator blacklist only kick in based on the return type being an iterator name though. That doesn't get expressed as easily with matchers (or IDK how atm). > > We could do that up here instead, but it'll be a separate list of methods still. Prefer that? Hmm... that makes it more complicated. I'd suggest putting it (including the iterator stuff) in a helper function like IsBlacklistedMethod() and calling that from here then. Or if you want to be really fancy, you can make it a custom matcher like isBlacklistedMethod().
Done PTAL (Also at the predecessor CL :))
lgtm https://codereview.chromium.org/1783923003/diff/20001/tools/clang/rewrite_to_... File tools/clang/rewrite_to_chrome_style/RewriteToChromeStyle.cpp (right): https://codereview.chromium.org/1783923003/diff/20001/tools/clang/rewrite_to_... tools/clang/rewrite_to_chrome_style/RewriteToChromeStyle.cpp:328: name = original_name.str(); Nit: get rid of |original_name| local here.
https://codereview.chromium.org/1783923003/diff/20001/tools/clang/rewrite_to_... File tools/clang/rewrite_to_chrome_style/RewriteToChromeStyle.cpp (right): https://codereview.chromium.org/1783923003/diff/20001/tools/clang/rewrite_to_... tools/clang/rewrite_to_chrome_style/RewriteToChromeStyle.cpp:328: name = original_name.str(); On 2016/03/10 23:57:42, dcheng wrote: > Nit: get rid of |original_name| local here. Done.
Rewrote as isBlacklistedFunction() matcher. PTAL
Description was changed from ========== rewrite_to_chrome_style: Blacklisted methods are not blink methods! Consider blacklisted methods to not be blink (or wtf) methods at all. This prevents us from ever trying to rewrite them, but also prevents us from asserting that we're going to try rewrite them but inheritance problems will result. R=dcheng BUG=593446 ========== to ========== rewrite_to_chrome_style: Remove blacklisted methods with matcher Consider blacklisted methods when looking at deciding to match against them or not rather than deciding to not rename them much later. R=dcheng BUG=593446 ==========
The CQ bit was checked by danakj@chromium.org to run a CQ dry run
Dry run: CQ is trying da patch. Follow status at https://chromium-cq-status.appspot.com/patch-status/1783923003/60001 View timeline at https://chromium-cq-status.appspot.com/patch-timeline/1783923003/60001
The CQ bit was unchecked by danakj@chromium.org
The CQ bit was checked by danakj@chromium.org
The patchset sent to the CQ was uploaded after l-g-t-m from dcheng@chromium.org Link to the patchset: https://codereview.chromium.org/1783923003/#ps60001 (title: "rewrite-blacklist-namespace: rewrite")
CQ is trying da patch. Follow status at https://chromium-cq-status.appspot.com/patch-status/1783923003/60001 View timeline at https://chromium-cq-status.appspot.com/patch-timeline/1783923003/60001
Message was sent while issue was closed.
Description was changed from ========== rewrite_to_chrome_style: Remove blacklisted methods with matcher Consider blacklisted methods when looking at deciding to match against them or not rather than deciding to not rename them much later. R=dcheng BUG=593446 ========== to ========== rewrite_to_chrome_style: Remove blacklisted methods with matcher Consider blacklisted methods when looking at deciding to match against them or not rather than deciding to not rename them much later. R=dcheng BUG=593446 ==========
Message was sent while issue was closed.
Committed patchset #4 (id:60001)
Message was sent while issue was closed.
Description was changed from ========== rewrite_to_chrome_style: Remove blacklisted methods with matcher Consider blacklisted methods when looking at deciding to match against them or not rather than deciding to not rename them much later. R=dcheng BUG=593446 ========== to ========== rewrite_to_chrome_style: Remove blacklisted methods with matcher Consider blacklisted methods when looking at deciding to match against them or not rather than deciding to not rename them much later. R=dcheng BUG=593446 Committed: https://crrev.com/27f1728f9c2cdfb9995c671783edd55bd66470be Cr-Commit-Position: refs/heads/master@{#380735} ==========
Message was sent while issue was closed.
Patchset 4 (id:??) landed as https://crrev.com/27f1728f9c2cdfb9995c671783edd55bd66470be Cr-Commit-Position: refs/heads/master@{#380735} |
