Chromium Code Reviews
|
|
Chromium Code Reviews
Issue
2929993003:
[api] Fix compilation error for `UNIMPLEMENTED()` method (Closed)
|
Created:
3 years, 6 months ago by Anna Henningsen Modified:
3 years, 6 months ago CC:
v8-reviews_googlegroups.com Target Ref:
refs/heads/master Project:
v8 Visibility:
Public. |
Description[api] Fix compilation error for `UNIMPLEMENTED()` method
Return `nullptr` from `ArrayBuffer::Allocator::Reserve` because
apparently not doing so results in compile errors for some people.
BUG=
Ref: https://github.com/nodejs/node/issues/13392
Review-Url: https://codereview.chromium.org/2929993003
Cr-Commit-Position: refs/heads/master@{#45886}
Committed: https://chromium.googlesource.com/v8/v8/+/f14d1b62313329f421f12cae429673c1b13018f9
Patch Set 1 #
Messages
Total messages: 23 (12 generated)
Description was changed from ========== [api] Fix compilation error for `UNIMPLEMENTED()` method Return `nullptr` from `ArrayBuffer::Allocator::Reserve` because apparently not doing so results in compile errors for some people. BUG= Ref: https://github.com/nodejs/node/issues/13392 ========== to ========== [api] Fix compilation error for `UNIMPLEMENTED()` method Return `nullptr` from `ArrayBuffer::Allocator::Reserve` because apparently not doing so results in compile errors for some people. BUG= Ref: https://github.com/nodejs/node/issues/13392 CQ_INCLUDE_TRYBOTS=master.tryserver.chromium.linux:linux_chromium_rel_ng ==========
addaleax@gmail.com changed reviewers: + franzih@google.com
The CQ bit was checked by franzih@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...
The CQ bit was unchecked by commit-bot@chromium.org
Dry run: This issue passed the CQ dry run.
The CQ bit was checked by addaleax@gmail.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
No L-G-T-M from a valid reviewer yet. CQ run can only be started 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.
The CQ bit was checked by franzih@chromium.org
lgtm Thanks!
CQ is trying da patch. Follow status at: https://chromium-cq-status.appspot.com/v2/patch-status/codereview.chromium.or...
CQ is committing da patch.
Bot data: {"patchset_id": 1, "attempt_start_ts": 1497340988502370, "parent_rev":
"91fb26ec83928d9b1fa3a71cac788634ba89d3b2", "commit_rev":
"f14d1b62313329f421f12cae429673c1b13018f9"}
Message was sent while issue was closed.
Description was changed from ========== [api] Fix compilation error for `UNIMPLEMENTED()` method Return `nullptr` from `ArrayBuffer::Allocator::Reserve` because apparently not doing so results in compile errors for some people. BUG= Ref: https://github.com/nodejs/node/issues/13392 CQ_INCLUDE_TRYBOTS=master.tryserver.chromium.linux:linux_chromium_rel_ng ========== to ========== [api] Fix compilation error for `UNIMPLEMENTED()` method Return `nullptr` from `ArrayBuffer::Allocator::Reserve` because apparently not doing so results in compile errors for some people. BUG= Ref: https://github.com/nodejs/node/issues/13392 Review-Url: https://codereview.chromium.org/2929993003 Cr-Commit-Position: refs/heads/master@{#45886} Committed: https://chromium.googlesource.com/v8/v8/+/f14d1b62313329f421f12cae429673c1b13... ==========
Message was sent while issue was closed.
Committed patchset #1 (id:1) as https://chromium.googlesource.com/v8/v8/+/f14d1b62313329f421f12cae429673c1b13...
Message was sent while issue was closed.
clemensh@chromium.org changed reviewers: + clemensh@chromium.org
Message was sent while issue was closed.
Compilers should be able to deduce that UNIMPLEMENTED() does not return, and should not complain. Can you check whether this CL would also have fixed the issue: https://chromium-review.googlesource.com/529072 (background: we require C++11 now, and all C++11-capable compilers should honour the [[noreturn]] annotation) If yes, we could revert this one.
Message was sent while issue was closed.
On 2017/06/13 at 14:31:39, Clemens Hammacher wrote: > Compilers should be able to deduce that UNIMPLEMENTED() does not return, and should not complain. > Can you check whether this CL would also have fixed the issue: https://chromium-review.googlesource.com/529072 > (background: we require C++11 now, and all C++11-capable compilers should honour the [[noreturn]] annotation) > > If yes, we could revert this one. Did we come to a conclusion here?
Message was sent while issue was closed.
On 2017/06/20 10:09:32, Clemens Hammacher wrote: > On 2017/06/13 at 14:31:39, Clemens Hammacher wrote: > > Compilers should be able to deduce that UNIMPLEMENTED() does not return, and > should not complain. > > Can you check whether this CL would also have fixed the issue: > https://chromium-review.googlesource.com/529072 > > (background: we require C++11 now, and all C++11-capable compilers should > honour the [[noreturn]] annotation) > > > > If yes, we could revert this one. > > Did we come to a conclusion here? Sorry, I tried to ping you on our corresponding Github thread but maybe I didn’t get your handle right: https://github.com/nodejs/node/pull/13634#issuecomment-308173874 It seems your CL also fixes the issue, so I see no reason to keep this patch (if you feel strongly enough – this is a temporary thing anyway).
Message was sent while issue was closed.
On 2017/06/20 at 17:00:44, addaleax wrote: > On 2017/06/20 10:09:32, Clemens Hammacher wrote: > > On 2017/06/13 at 14:31:39, Clemens Hammacher wrote: > > > Compilers should be able to deduce that UNIMPLEMENTED() does not return, and > > should not complain. > > > Can you check whether this CL would also have fixed the issue: > > https://chromium-review.googlesource.com/529072 > > > (background: we require C++11 now, and all C++11-capable compilers should > > honour the [[noreturn]] annotation) > > > > > > If yes, we could revert this one. > > > > Did we come to a conclusion here? > > Sorry, I tried to ping you on our corresponding Github thread but maybe I didn’t get your handle right: https://github.com/nodejs/node/pull/13634#issuecomment-308173874 > > It seems your CL also fixes the issue, so I see no reason to keep this patch (if you feel strongly enough – this is a temporary thing anyway). We had a cleanup recently to remove all returns after UNREACHABLE: https://chromium-review.googlesource.com/c/507287/ This CL goes exactly in the opposite direction, that's why I don't like it. I will create the revert now but wait for franzih to approve it.
Message was sent while issue was closed.
A revert of this CL (patchset #1 id:1) has been created in https://codereview.chromium.org/2946933002/ by clemensh@chromium.org. The reason for reverting is: not needed any more, and contradicts our cleanup efforts: https://chromium-review.googlesource.com/c/507287/. |
||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||
