Chromium Code Reviews
|
|
Chromium Code Reviews
Issue
2713403002:
Fix to suppress compiler unused-variable error (Closed)
|
Created:
3 years, 10 months ago by JaideepBajwa Modified:
3 years, 9 months ago CC:
v8-merges_googlegroups.com, v8-reviews_googlegroups.com Target Ref:
refs/pending/branch-heads/5.7 Project:
v8 Visibility:
Public. |
DescriptionFix to suppress compiler unused-variable error
R=bradnelson@chromium.org, hablich@chromium.org, machenbach@chromium.org, mtrofin@chromium.org
BUG=v8:5981
LOG=N
Review-Url: https://codereview.chromium.org/2713403002 .
Cr-Commit-Position: refs/branch-heads/5.7@{#134}
Cr-Branched-From: 975e9a320b6eaf9f12280c35df98e013beb8f041-refs/heads/5.7.492@{#1}
Cr-Branched-From: 8d76f0e3465a84bbf0bceab114900fbe75844e1f-refs/heads/master@{#42426}
Committed: https://chromium.googlesource.com/v8/v8/+/6bb221b49ebcac13653477d2d3e01f921a721474
Patch Set 1 #
Messages
Total messages: 41 (16 generated)
ptal, Please see https://codereview.chromium.org/2709613008/ for more details.
Description was changed from ========== Fix to suppress compiler unused-variable error R=mtrofin@chromium.org, bradnelson@chromium.org, machenbach@chromium.org BUG= LOG=N ========== to ========== Fix to suppress compiler unused-variable error R=mtrofin@chromium.org, bradnelson@chromium.org, machenbach@chromium.org BUG= LOG=N ==========
mtrofin@chromium.org changed required reviewers: + machenbach@chromium.org
The CQ bit was checked by bradnelson@chromium.org
lgtm
CQ is trying da patch. Follow status at https://chromium-cq-status.appspot.com/v2/patch-status/codereview.chromium.or...
The CQ bit was unchecked by commit-bot@chromium.org
All required reviewers (with asterisk prefixes) have not yet approved this CL. No L-G-T-M from a valid reviewer yet. CQ run can only be started by full committers or once the patch has received an L-G-T-M from a full committer. Even if an L-G-T-M may have been provided, it was from a non-committer, _not_ a full super star committer. Committers are members of the group "project-v8-committers". Note that this has nothing to do with OWNERS files.
Actually, I just noticed this is on the branch. Please land on trunk first.
On 2017/02/27 04:29:54, bradnelson wrote: > Actually, I just noticed this is on the branch. > Please land on trunk first. Would a gyp-side extra cflag flag to switch off this particular warning help? If yes, that'd be preferred to a code-change.
On 2017/02/27 04:29:54, bradnelson wrote: > Actually, I just noticed this is on the branch. > Please land on trunk first. Having "USE(bytes)" would be redundant on truck as bytes is already being used under CL https://codereview.chromium.org/2695813005/. Should I still land it there?
On 2017/02/27 07:51:34, Michael Achenbach wrote: > On 2017/02/27 04:29:54, bradnelson wrote: > > Actually, I just noticed this is on the branch. > > Please land on trunk first. > > Would a gyp-side extra cflag flag to switch off this particular warning help? If > yes, that'd be preferred to a code-change. -Wunused-variable should suppress this error, but not sure if that is preferred because USE() template was added just for this specific compiler warning and is used at many places, so I suppose the idea was to catch these warning?
machenbach@chromium.org changed reviewers: + hablich@chromium.org
machenbach@chromium.org changed required reviewers: + hablich@chromium.org
Probably fine. LGTM. I leave it to hablich to handle our merge procedure.
On 2017/02/28 at 16:05:23, machenbach wrote: > Probably fine. LGTM. I leave it to hablich to handle our merge procedure. lgtm
I think you mentioned that this is PPC only? Please add the [PPC] prefix to the title.
Description was changed from ========== Fix to suppress compiler unused-variable error R=mtrofin@chromium.org, bradnelson@chromium.org, machenbach@chromium.org BUG= LOG=N ========== to ========== [PPC] Fix to suppress compiler unused-variable error R=mtrofin@chromium.org, bradnelson@chromium.org, machenbach@chromium.org BUG= LOG=N ==========
I suppose it would also make sense to link the WASM issue?
Description was changed from ========== [PPC] Fix to suppress compiler unused-variable error R=mtrofin@chromium.org, bradnelson@chromium.org, machenbach@chromium.org BUG= LOG=N ========== to ========== [PPC] Fix to suppress compiler unused-variable error R=mtrofin@chromium.org, bradnelson@chromium.org, machenbach@chromium.org BUG=v8:5981 LOG=N ==========
bradnelson@google.com changed reviewers: + bradnelson@google.com
Linked to that bug.
On 2017/02/28 19:00:11, Michael Hablich wrote: > I think you mentioned that this is PPC only? Please add the [PPC] prefix to the > title. Its not necessarily PPC related, even x64 built with gyp/make fails. And projects like Node.js using gyp/make would also fail when it moves to 5.7
Description was changed from ========== [PPC] Fix to suppress compiler unused-variable error R=mtrofin@chromium.org, bradnelson@chromium.org, machenbach@chromium.org BUG=v8:5981 LOG=N ========== to ========== Fix to suppress compiler unused-variable error R=mtrofin@chromium.org, bradnelson@chromium.org, machenbach@chromium.org BUG=v8:5981 LOG=N ==========
On 2017/02/28 at 19:38:59, bjaideep wrote: > On 2017/02/28 19:00:11, Michael Hablich wrote: > > I think you mentioned that this is PPC only? Please add the [PPC] prefix to the > > title. > > Its not necessarily PPC related, even x64 built with gyp/make fails. And projects like > Node.js using gyp/make would also fail when it moves to 5.7 IC, thanks for clarifying. still LGTM IMO.
On 2017/02/28 19:38:59, JaideepBajwa wrote: > On 2017/02/28 19:00:11, Michael Hablich wrote: > > I think you mentioned that this is PPC only? Please add the [PPC] prefix to > the > > title. > > Its not necessarily PPC related, even x64 built with gyp/make fails. And > projects like > Node.js using gyp/make would also fail when it moves to 5.7 I assume this is why https://build.chromium.org/p/client.v8.branches/builders/V8%20ppc%20-%20sim%2... failes? Note that this is not gyp/make.
The CQ bit was checked by bradnelson@google.com
CQ is trying da patch. Follow status at https://chromium-cq-status.appspot.com/v2/patch-status/codereview.chromium.or...
The CQ bit was unchecked by commit-bot@chromium.org
CLs for remote refs other than refs/heads/master must contain NOTRY=true and NOPRESUBMIT=true in order for the CQ to process them
Description was changed from ========== Fix to suppress compiler unused-variable error R=mtrofin@chromium.org, bradnelson@chromium.org, machenbach@chromium.org BUG=v8:5981 LOG=N ========== to ========== Fix to suppress compiler unused-variable error NOTRY=true NOPRESUBMIT=true R=mtrofin@chromium.org, bradnelson@chromium.org, machenbach@chromium.org BUG=v8:5981 LOG=N ==========
Description was changed from ========== Fix to suppress compiler unused-variable error NOTRY=true NOPRESUBMIT=true R=mtrofin@chromium.org, bradnelson@chromium.org, machenbach@chromium.org BUG=v8:5981 LOG=N ========== to ========== Fix to suppress compiler unused-variable error R=mtrofin@chromium.org, bradnelson@chromium.org, machenbach@chromium.org BUG=v8:5981 LOG=N ==========
Why'd you change the description back and forth? Please keep it with NOTRY and NOPRESUBMIT. That's by design as the branch CQ doesn't support trybots. This can land as is.
On 2017/02/28 20:13:04, Michael Achenbach wrote: > Why'd you change the description back and forth? Please keep it with NOTRY and > NOPRESUBMIT. That's by design as the branch CQ doesn't support trybots. This can > land as is. Ah, and this comment might be confusing unless you "Show Generated Messages" :)
Description was changed from ========== Fix to suppress compiler unused-variable error R=mtrofin@chromium.org, bradnelson@chromium.org, machenbach@chromium.org BUG=v8:5981 LOG=N ========== to ========== Fix to suppress compiler unused-variable error R=bradnelson@chromium.org, hablich@chromium.org, machenbach@chromium.org, mtrofin@chromium.org BUG=v8:5981 LOG=N Review-Url: https://codereview.chromium.org/2713403002 . Cr-Commit-Position: refs/branch-heads/5.7@{#134} Cr-Branched-From: 975e9a320b6eaf9f12280c35df98e013beb8f041-refs/heads/5.7.492@{#1} Cr-Branched-From: 8d76f0e3465a84bbf0bceab114900fbe75844e1f-refs/heads/master@{#42426} Committed: https://chromium.googlesource.com/v8/v8/+/6bb221b49ebcac13653477d2d3e01f921a7... ==========
Message was sent while issue was closed.
Committed patchset #1 (id:1) manually as 6bb221b49ebcac13653477d2d3e01f921a721474 (presubmit successful).
Message was sent while issue was closed.
Landed at: https://chromium.googlesource.com/v8/v8/+/6bb221b49ebcac13653477d2d3e01f921a7...
Message was sent while issue was closed.
Description was changed from ========== Fix to suppress compiler unused-variable error R=bradnelson@chromium.org, hablich@chromium.org, machenbach@chromium.org, mtrofin@chromium.org BUG=v8:5981 LOG=N Review-Url: https://codereview.chromium.org/2713403002 . Cr-Commit-Position: refs/branch-heads/5.7@{#134} Cr-Branched-From: 975e9a320b6eaf9f12280c35df98e013beb8f041-refs/heads/5.7.492@{#1} Cr-Branched-From: 8d76f0e3465a84bbf0bceab114900fbe75844e1f-refs/heads/master@{#42426} Committed: https://chromium.googlesource.com/v8/v8/+/6bb221b49ebcac13653477d2d3e01f921a7... ========== to ========== Fix to suppress compiler unused-variable error R=bradnelson@chromium.org, hablich@chromium.org, machenbach@chromium.org, mtrofin@chromium.org BUG=v8:5981 LOG=N Review-Url: https://codereview.chromium.org/2713403002 . Cr-Commit-Position: refs/branch-heads/5.7@{#134} Cr-Branched-From: 975e9a320b6eaf9f12280c35df98e013beb8f041-refs/heads/5.7.492@{#1} Cr-Branched-From: 8d76f0e3465a84bbf0bceab114900fbe75844e1f-refs/heads/master@{#42426} Committed: https://chromium.googlesource.com/v8/v8/+/6bb221b49ebcac13653477d2d3e01f921a7... ==========
Message was sent while issue was closed.
On 2017/02/28 19:56:34, Michael Achenbach wrote: > On 2017/02/28 19:38:59, JaideepBajwa wrote: > > On 2017/02/28 19:00:11, Michael Hablich wrote: > > > I think you mentioned that this is PPC only? Please add the [PPC] prefix to > > the > > > title. > > > > Its not necessarily PPC related, even x64 built with gyp/make fails. And > > projects like > > Node.js using gyp/make would also fail when it moves to 5.7 > > I assume this is why > https://build.chromium.org/p/client.v8.branches/builders/V8%20ppc%20-%20sim%2... > failes? Note that this is not gyp/make. Actually its looks like for ppc/s390 sim builds, it still uses gyp configs not exactly sure how it works with ninja (haven't looked into details) but in infra/mb/mb_config.pyl, under 'client.v8.branches', only ppc/s390 has "gyp_release_.., rest architecture have "gn_release_..". I will look into what changes needs to be made for ppc/s390 as I'm aware that gyp/make will no longer be supported by end of the year.
Message was sent while issue was closed.
On 2017/02/28 20:17:43, bradnelson wrote: > Committed patchset #1 (id:1) manually as > 6bb221b49ebcac13653477d2d3e01f921a721474 (presubmit successful). Thanks. |
||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||
