Chromium Code Reviews
|
|
Chromium Code Reviews
Issue
1589633007:
[WATCHLISTS] CC v8-$arch-ports automatically on platform-specific CLs (Closed)
|
Created:
4 years, 11 months ago by Jakob Kummerow Modified:
4 years, 11 months ago CC:
v8-mips-ports_googlegroups.com, v8-ppc-ports_googlegroups.com, v8-reviews_googlegroups.com, v8-x87-ports_googlegroups.com Base URL:
https://chromium.googlesource.com/v8/v8.git@master Target Ref:
refs/pending/heads/master Project:
v8 Visibility:
Public. |
Description[WATCHLISTS] CC v8-$arch-ports automatically on platform-specific CLs
Automate ALL the repetitive workflows!
Drive-by: fix 'interpreter' watchlist definition.
NOTRY=true
Committed: https://crrev.com/009b3514273a15475388c07422a5c748a2a873c6
Cr-Commit-Position: refs/heads/master@{#33383}
Patch Set 1 #
Total comments: 3
Messages
Total messages: 16 (6 generated)
jkummerow@chromium.org changed reviewers: + cbruni@chromium.org
Core V8 team, this should save you some typing. Friends of MIPS/PPC/x87, do you have suggestions for improving the triggers? Have I forgotten any lists?
On 2016/01/15 12:29:29, Jakob wrote: > Core V8 team, this should save you some typing. > > Friends of MIPS/PPC/x87, do you have suggestions for improving the triggers? > > Have I forgotten any lists? Interpreter changes lgtm, thanks!
hablich@chromium.org changed reviewers: + hablich@chromium.org
I would also add the other ports like x87, ppc and mips(64) to the list. https://codereview.chromium.org/1589633007/diff/1/WATCHLISTS File WATCHLISTS (right): https://codereview.chromium.org/1589633007/diff/1/WATCHLISTS#newcode57 WATCHLISTS:57: 'filepath': '/arm/', shouldn't it be 'src/arm'? https://codereview.chromium.org/1589633007/diff/1/WATCHLISTS#newcode60 WATCHLISTS:60: 'filepath': '/ia32/', same as above
On 2016/01/15 14:15:42, Hablich wrote: > I would also add the other ports like x87, ppc and mips(64) to the list. I wouldn't add unused definitions :-) https://codereview.chromium.org/1589633007/diff/1/WATCHLISTS File WATCHLISTS (right): https://codereview.chromium.org/1589633007/diff/1/WATCHLISTS#newcode57 WATCHLISTS:57: 'filepath': '/arm/', On 2016/01/15 14:15:42, Hablich wrote: > shouldn't it be 'src/arm'? Nope, it should match any ".../foo/arm/bar/..." path.
paul.lind@imgtec.com changed reviewers: + paul.lind@imgtec.com
The trigger for mips lgtm. (though we sometimes steal ideas from arm64 as well :) Sometimes Core v8 team does mips ports, and sometimes requests us to do them. Typically, CC occurred only for porting-requests. It would be nice to clarify when the CC is just informative, when a code-review is requested, and when porting is requested. I don't want to duplicate effort, nor do I want to slow the Core team if/when a response from us is desired.
> The trigger for mips lgtm. (though we sometimes steal ideas from arm64 as well
> :)
Well, of course you're free to look at any arch you want. And of course there is
a chance that an arm64-only CL requires a mips or mips64 port -- but that case
should be much rarer than CLs touching all platforms, I think.
> Sometimes Core v8 team does mips ports, and sometimes requests us to do them.
> Typically, CC occurred only for porting-requests.
>
> It would be nice to clarify when the CC is just informative, when a
code-review
> is requested, and when porting is requested. I don't want to duplicate effort,
> nor do I want to slow the Core team if/when a response from us is desired.
Good point. Automated CC doesn't necessarily convey intent and hence can't fully
replace human communication. Suggested plan of action:
(1) Hope that it's not an issue, i.e. that people will either put appropriate
comments ("MIPS: FYI" or "MIPS: please port") into the CL message, or delete the
auto-populated CC, or the situation is obvious (when you see a CL with a
complete MIPS port, presumably you wouldn't robotically port it again, but
instead maybe review the MIPS bits or just ignore the mail).
(2) If (1) doesn't hold, send out a PSA to remind people of good practices.
(3) If (2) doesn't help, revert this change.
cbruni@chromium.org changed reviewers: + rmcilroy@chromium.org
lgtm
The CQ bit was checked by jkummerow@chromium.org
CQ is trying da patch. Follow status at https://chromium-cq-status.appspot.com/patch-status/1589633007/1 View timeline at https://chromium-cq-status.appspot.com/patch-timeline/1589633007/1
Message was sent while issue was closed.
Committed patchset #1 (id:1)
Message was sent while issue was closed.
Description was changed from ========== [WATCHLISTS] CC v8-$arch-ports automatically on platform-specific CLs Automate ALL the repetitive workflows! Drive-by: fix 'interpreter' watchlist definition. NOTRY=true ========== to ========== [WATCHLISTS] CC v8-$arch-ports automatically on platform-specific CLs Automate ALL the repetitive workflows! Drive-by: fix 'interpreter' watchlist definition. NOTRY=true Committed: https://crrev.com/009b3514273a15475388c07422a5c748a2a873c6 Cr-Commit-Position: refs/heads/master@{#33383} ==========
Message was sent while issue was closed.
Patchset 1 (id:??) landed as https://crrev.com/009b3514273a15475388c07422a5c748a2a873c6 Cr-Commit-Position: refs/heads/master@{#33383} |
||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||
