Chromium Code Reviews
chromiumcodereview-hr@appspot.gserviceaccount.com (chromiumcodereview-hr) | Please choose your nickname with Settings | Help | Chromium Project | Gerrit Changes | Sign out
(87)

Issue 1470983002: Make SerialIoHandlerWin::UiThreadHelper final. (Closed)

Created:
5 years, 1 month ago by Nico
Modified:
5 years, 1 month ago
Reviewers:
juncai
CC:
chromium-reviews, grt (UTC plus 2), hans
Base URL:
https://chromium.googlesource.com/chromium/src.git@master
Target Ref:
refs/pending/heads/master
Project:
chromium
Visibility:
Public.

Description

Make SerialIoHandlerWin::UiThreadHelper final. Fixes the clang/win build after https://codereview.chromium.org/1439443002/ clang rightfully complains that UiThreadHelper has virtual methods, is deleted polymorphically, and doesn't have a virtual destructor: ..\..\base/sequenced_task_runner_helpers.h(40,5) : error: delete called on 'const device::SerialIoHandlerWin::UiThreadHelper' that has virtual functions but non-virtual destructor [-Werror,-Wdelete-non-virtual-dtor] delete reinterpret_cast<const T*>(object); ^ ..\..\base/sequenced_task_runner_helpers.h(86,38) : note: in instantiation of member function 'base::DeleteHelper<device::SerialIoHandlerWin::UiThreadHelper>::DoDelete' requested here from_here, &DeleteHelper<T>::DoDelete, object); ^ ..\..\base/sequenced_task_runner.h(126,48) : note: in instantiation of function template specialization 'base::subtle::DeleteHelperInternal<device::SerialIoHandlerWin::UiThreadHelper, bool>::DeleteViaSequencedTaskRunner<base::SequencedTaskRunner>' requested here subtle::DeleteHelperInternal<T, bool>::DeleteViaSequencedTaskRunner( ^ ..\..\device\serial\serial_io_handler_win.cc(374,28) : note: in instantiation of function template specialization 'base::SequencedTaskRunner::DeleteSoon<device::SerialIoHandlerWin::UiThreadHelper>' requested here ui_thread_task_runner()->DeleteSoon(FROM_HERE, helper_); ^ Making the class final fixes this as it makes sure that nobody adds a subclass of UiThreadHelper. Also don't mix initializer styles for the different fields of this class. No intended behavior change. BUG=82385 TBR=juncai Committed: https://crrev.com/db730725a86c540c8d9ba7d77c9651eb35389469 Cr-Commit-Position: refs/heads/master@{#361117}

Patch Set 1 #

Unified diffs Side-by-side diffs Delta from patch set Stats (+4 lines, -2 lines) Patch
M device/serial/serial_io_handler_win.h View 1 chunk +1 line, -1 line 0 comments Download
M device/serial/serial_io_handler_win.cc View 2 chunks +3 lines, -1 line 0 comments Download

Messages

Total messages: 9 (4 generated)
Nico
5 years, 1 month ago (2015-11-23 15:58:01 UTC) #4
commit-bot: I haz the power
CQ is trying da patch. Follow status at https://chromium-cq-status.appspot.com/patch-status/1470983002/1 View timeline at https://chromium-cq-status.appspot.com/patch-timeline/1470983002/1
5 years, 1 month ago (2015-11-23 15:58:57 UTC) #6
Nico
Committed patchset #1 (id:1) manually as db730725a86c540c8d9ba7d77c9651eb35389469 (presubmit successful).
5 years, 1 month ago (2015-11-23 16:24:10 UTC) #7
commit-bot: I haz the power
Patchset 1 (id:??) landed as https://crrev.com/db730725a86c540c8d9ba7d77c9651eb35389469 Cr-Commit-Position: refs/heads/master@{#361117}
5 years, 1 month ago (2015-11-23 16:24:14 UTC) #8
juncai
5 years, 1 month ago (2015-11-23 17:36:18 UTC) #9
Message was sent while issue was closed.
On 2015/11/23 15:58:01, Nico wrote:

lgtm, thanks!

Powered by Google App Engine
This is Rietveld 408576698