Chromium Code Reviews
|
|
Chromium Code Reviews
Issue
1735383002:
Try fix clang/win builds (Closed)
DescriptionTry fix clang/win build broken by https://codereview.chromium.org/1643573002/
BUG=82385
Committed: https://crrev.com/e0a95c8765a7071cf7dc70746000d0a378d00138
Cr-Commit-Position: refs/heads/master@{#377691}
Patch Set 1 #
Total comments: 1
Patch Set 2 : Remove override #
Total comments: 2
Patch Set 3 : Remove virtual instead of override #Messages
Total messages: 16 (6 generated)
Description was changed from ========== Try fix clang/win builds BUG= ========== to ========== Try fix clang/win build broken by https://codereview.chromium.org/1643573002/ BUG= ==========
thakis@chromium.org changed reviewers: + thakis@chromium.org
https://codereview.chromium.org/1735383002/diff/1/chrome/browser/safe_browsin... File chrome/browser/safe_browsing/incident_reporting/module_load_analyzer_win_unittest.cc (right): https://codereview.chromium.org/1735383002/diff/1/chrome/browser/safe_browsin... chrome/browser/safe_browsing/incident_reporting/module_load_analyzer_win_unittest.cc:45: virtual ~MockSafeBrowsingDatabaseManager() override {} remove virtual, else the style plugin will complain that you have both virtual and override
On 2016/02/25 21:11:29, Nico wrote: > https://codereview.chromium.org/1735383002/diff/1/chrome/browser/safe_browsin... > File > chrome/browser/safe_browsing/incident_reporting/module_load_analyzer_win_unittest.cc > (right): > > https://codereview.chromium.org/1735383002/diff/1/chrome/browser/safe_browsin... > chrome/browser/safe_browsing/incident_reporting/module_load_analyzer_win_unittest.cc:45: > virtual ~MockSafeBrowsingDatabaseManager() override {} > remove virtual, else the style plugin will complain that you have both virtual > and override Actually removed the override, since that seems to work for https://code.google.com/p/chromium/codesearch#chromium/src/chrome/browser/saf...
https://codereview.chromium.org/1735383002/diff/20001/chrome/browser/safe_bro... File chrome/browser/safe_browsing/incident_reporting/module_load_analyzer_win_unittest.cc (right): https://codereview.chromium.org/1735383002/diff/20001/chrome/browser/safe_bro... chrome/browser/safe_browsing/incident_reporting/module_load_analyzer_win_unittest.cc:45: virtual ~MockSafeBrowsingDatabaseManager() {} You removed the wrong one. TestSafeBrowsingDatabaseManager does have a virtual dtor, so this does override something. Now the style plugin will complain that this needs an override.
On Thu, Feb 25, 2016 at 4:16 PM, <proberge@chromium.org> wrote: > Reviewers: Nico > CL: https://codereview.chromium.org/1735383002/ > > Message: > On 2016/02/25 21:11:29, Nico wrote: > > > > https://codereview.chromium.org/1735383002/diff/1/chrome/browser/safe_browsin... > > File > > > > chrome/browser/safe_browsing/incident_reporting/module_load_analyzer_win_unittest.cc > > (right): > > > > > > https://codereview.chromium.org/1735383002/diff/1/chrome/browser/safe_browsin... > > > > chrome/browser/safe_browsing/incident_reporting/module_load_analyzer_win_unittest.cc:45: > > virtual ~MockSafeBrowsingDatabaseManager() override {} > > remove virtual, else the style plugin will complain that you have both > virtual > > and override > > Actually removed the override, since that seems to work for > > https://code.google.com/p/chromium/codesearch#chromium/src/chrome/browser/saf... > Wow, that's a bit disconcerting, that's not supposed to work :-) Let me file a bug for that. > > > Description: > Try fix clang/win build broken by > https://codereview.chromium.org/1643573002/ > > BUG= > > Base URL: https://chromium.googlesource.com/chromium/src.git@master > > Affected files (+2, -0 lines): > M > chrome/browser/safe_browsing/incident_reporting/module_load_analyzer_win_unittest.cc > > > Index: > chrome/browser/safe_browsing/incident_reporting/module_load_analyzer_win_unittest.cc > diff --git > a/chrome/browser/safe_browsing/incident_reporting/module_load_analyzer_win_unittest.cc > b/chrome/browser/safe_browsing/incident_reporting/module_load_analyzer_win_unittest.cc > index > 3422794aefd7f752f7d6f7e637985535c6120c96..25150e930e60a2ca06fb259debd43271296e51f8 > 100644 > --- > a/chrome/browser/safe_browsing/incident_reporting/module_load_analyzer_win_unittest.cc > +++ > b/chrome/browser/safe_browsing/incident_reporting/module_load_analyzer_win_unittest.cc > @@ -42,6 +42,8 @@ class MockSafeBrowsingDatabaseManager : public > TestSafeBrowsingDatabaseManager { > MOCK_METHOD1(MatchModuleWhitelistString, bool(const std::string&)); > > private: > + virtual ~MockSafeBrowsingDatabaseManager() {} > + > DISALLOW_COPY_AND_ASSIGN(MockSafeBrowsingDatabaseManager); > }; > > > > -- You received this message because you are subscribed to the Google Groups "Chromium-reviews" group. To unsubscribe from this group and stop receiving emails from it, send an email to chromium-reviews+unsubscribe@chromium.org.
On Thu, Feb 25, 2016 at 4:18 PM, Nico Weber <thakis@chromium.org> wrote: > On Thu, Feb 25, 2016 at 4:16 PM, <proberge@chromium.org> wrote: > >> Reviewers: Nico >> CL: https://codereview.chromium.org/1735383002/ >> >> Message: >> On 2016/02/25 21:11:29, Nico wrote: >> > >> >> https://codereview.chromium.org/1735383002/diff/1/chrome/browser/safe_browsin... >> > File >> > >> >> chrome/browser/safe_browsing/incident_reporting/module_load_analyzer_win_unittest.cc >> > (right): >> > >> > >> >> https://codereview.chromium.org/1735383002/diff/1/chrome/browser/safe_browsin... >> > >> >> chrome/browser/safe_browsing/incident_reporting/module_load_analyzer_win_unittest.cc:45: >> > virtual ~MockSafeBrowsingDatabaseManager() override {} >> > remove virtual, else the style plugin will complain that you have both >> virtual >> > and override >> >> Actually removed the override, since that seems to work for >> >> https://code.google.com/p/chromium/codesearch#chromium/src/chrome/browser/saf... >> > > Wow, that's a bit disconcerting, that's not supposed to work :-) Let me > file a bug for that. > https://bugs.chromium.org/p/chromium/issues/detail?id=589969 > > >> >> >> Description: >> Try fix clang/win build broken by >> https://codereview.chromium.org/1643573002/ >> >> BUG= >> >> Base URL: https://chromium.googlesource.com/chromium/src.git@master >> >> Affected files (+2, -0 lines): >> M >> chrome/browser/safe_browsing/incident_reporting/module_load_analyzer_win_unittest.cc >> >> >> Index: >> chrome/browser/safe_browsing/incident_reporting/module_load_analyzer_win_unittest.cc >> diff --git >> a/chrome/browser/safe_browsing/incident_reporting/module_load_analyzer_win_unittest.cc >> b/chrome/browser/safe_browsing/incident_reporting/module_load_analyzer_win_unittest.cc >> index >> 3422794aefd7f752f7d6f7e637985535c6120c96..25150e930e60a2ca06fb259debd43271296e51f8 >> 100644 >> --- >> a/chrome/browser/safe_browsing/incident_reporting/module_load_analyzer_win_unittest.cc >> +++ >> b/chrome/browser/safe_browsing/incident_reporting/module_load_analyzer_win_unittest.cc >> @@ -42,6 +42,8 @@ class MockSafeBrowsingDatabaseManager : public >> TestSafeBrowsingDatabaseManager { >> MOCK_METHOD1(MatchModuleWhitelistString, bool(const std::string&)); >> >> private: >> + virtual ~MockSafeBrowsingDatabaseManager() {} >> + >> DISALLOW_COPY_AND_ASSIGN(MockSafeBrowsingDatabaseManager); >> }; >> >> >> >> > -- You received this message because you are subscribed to the Google Groups "Chromium-reviews" group. To unsubscribe from this group and stop receiving emails from it, send an email to chromium-reviews+unsubscribe@chromium.org.
https://codereview.chromium.org/1735383002/diff/20001/chrome/browser/safe_bro... File chrome/browser/safe_browsing/incident_reporting/module_load_analyzer_win_unittest.cc (right): https://codereview.chromium.org/1735383002/diff/20001/chrome/browser/safe_bro... chrome/browser/safe_browsing/incident_reporting/module_load_analyzer_win_unittest.cc:45: virtual ~MockSafeBrowsingDatabaseManager() {} On 2016/02/25 21:17:08, Nico wrote: > You removed the wrong one. TestSafeBrowsingDatabaseManager does have a virtual > dtor, so this does override something. Now the style plugin will complain that > this needs an override. Done.
lgtm, thanks!
The CQ bit was checked by thakis@chromium.org
Description was changed from ========== Try fix clang/win build broken by https://codereview.chromium.org/1643573002/ BUG= ========== to ========== Try fix clang/win build broken by https://codereview.chromium.org/1643573002/ BUG=82385 ==========
CQ is trying da patch. Follow status at https://chromium-cq-status.appspot.com/patch-status/1735383002/40001 View timeline at https://chromium-cq-status.appspot.com/patch-timeline/1735383002/40001
Message was sent while issue was closed.
Description was changed from ========== Try fix clang/win build broken by https://codereview.chromium.org/1643573002/ BUG=82385 ========== to ========== Try fix clang/win build broken by https://codereview.chromium.org/1643573002/ BUG=82385 ==========
Message was sent while issue was closed.
Committed patchset #3 (id:40001)
Message was sent while issue was closed.
Description was changed from ========== Try fix clang/win build broken by https://codereview.chromium.org/1643573002/ BUG=82385 ========== to ========== Try fix clang/win build broken by https://codereview.chromium.org/1643573002/ BUG=82385 Committed: https://crrev.com/e0a95c8765a7071cf7dc70746000d0a378d00138 Cr-Commit-Position: refs/heads/master@{#377691} ==========
Message was sent while issue was closed.
Patchset 3 (id:??) landed as https://crrev.com/e0a95c8765a7071cf7dc70746000d0a378d00138 Cr-Commit-Position: refs/heads/master@{#377691} |
