Chromium Code Reviews
|
|
Chromium Code Reviews
Issue
1567983003:
call static SequencedWorkerPool::GetSequenceToken() method directly (Closed)
|
Created:
4 years, 11 months ago by Mostyn Bramley-Moore Modified:
4 years, 11 months ago CC:
cbentzel+watch_chromium.org, chromium-reviews, darin-cc_chromium.org, jam, jkarlin+watch_chromium.org, kalyank, nhiroki, sadrul Base URL:
https://chromium.googlesource.com/chromium/src.git@master Target Ref:
refs/pending/heads/master Project:
chromium Visibility:
Public. |
Descriptioncall static SequencedWorkerPool::GetSequenceToken() method directly
Followup to https://codereview.chromium.org/1414793009/ in order
to avoid a GCC unused result warning.
SequencedWorkerPool::GetSequenceToken() is now a static method, GCC
emits a warning for BrowserThread::GetBlockingPool()->GetSequenceToken()
since the return value of BrowserThread::GetBlockingPool() is not
used. We should just all the static method directly.
Committed: https://crrev.com/c965fd6ce9d24bf6ed897266c771cc9c6f20c1ef
Cr-Commit-Position: refs/heads/master@{#368326}
Patch Set 1 #Patch Set 2 : drop WARN_UNUSED_RESULT from GetBlockingPool() #Patch Set 3 : drop WARN_UNUSED_RESULT change and call static method directly #
Messages
Total messages: 31 (14 generated)
The CQ bit was checked by mostynb@opera.com to run a CQ dry run
Dry run: CQ is trying da patch. Follow status at https://chromium-cq-status.appspot.com/patch-status/1567983003/1 View timeline at https://chromium-cq-status.appspot.com/patch-timeline/1567983003/1
mostynb@opera.com changed reviewers: + bauerb@chromium.org, danakj@chromium.org, gab@chromium.org, jochen@chromium.org
Bernhard, danakj, gab: please take a look at this followup to https://codereview.chromium.org/1414793009/ @Jochen: PTAL, since you're an OWNER for each of these files. This feels like a GCC bug to me, but I have not had time to produce a minimised testcase to report to GCC yet. (IIRC there are still some chromium-based google products that use GCC.)
Description was changed from ========== work around GCC warn_unused_result strictness(/bug?) Followup to https://codereview.chromium.org/1414793009/ which works around an apparent GCC bug. BrowserThread::GetBlockingPool() is used in several places as part of a method call chain, and GCC emits a warning that the result is unused. -Werror promotes these warnings to errors. By simply storing the result from BrowserThread::GetBlockingPool() in an intermediate variable, the warnings can be avoided. ========== to ========== work around GCC warn_unused_result strictness(/bug?) Followup to https://codereview.chromium.org/1414793009/. This is a workaround for an apparent GCC bug. BrowserThread::GetBlockingPool() is used in several places as part of a method call chain, and GCC emits a warning that the result is unused. -Werror promotes these warnings to errors. By simply storing the result from BrowserThread::GetBlockingPool() in an intermediate variable, the warnings can be avoided. ==========
The CQ bit was unchecked by commit-bot@chromium.org
Dry run: Try jobs failed on following builders: win_chromium_x64_rel_ng on tryserver.chromium.win (JOB_FAILED, http://build.chromium.org/p/tryserver.chromium.win/builders/win_chromium_x64_...)
mostynb@opera.com changed reviewers: + thestig@chromium.org
Argh, patchset 1 hits an overenthusiastic windows toolchain warning in plugin_service_impl.cc (visual studio doesn't count calling a static method via a variable as referencing the variable). An alternative solution would be to drop WARN_UNUSED_RESULT from BrowserThread::GetBlockingPool(). It looks like this was added in https://codereview.chromium.org/250653002 to avoid forgetting to wrap calls to some functions in DCHECK(), but I can't find any instances of BrowserThread::GetBlockingPool() being DCHECK'ed. @Lei Zhang: does this look OK?
Description was changed from ========== work around GCC warn_unused_result strictness(/bug?) Followup to https://codereview.chromium.org/1414793009/. This is a workaround for an apparent GCC bug. BrowserThread::GetBlockingPool() is used in several places as part of a method call chain, and GCC emits a warning that the result is unused. -Werror promotes these warnings to errors. By simply storing the result from BrowserThread::GetBlockingPool() in an intermediate variable, the warnings can be avoided. ========== to ========== drop useless warn_unused_result attribute from BrowserThread::GetBlockingPool() Followup to https://codereview.chromium.org/1414793009/. This is a workaround for an apparent GCC bug. BrowserThread::GetBlockingPool() is used in several places as part of a method call chain, and GCC emits a warning that the result is unused. -Werror promotes these warnings to errors. WARN_UNUSED_RESULT was added to BrowserThread::GetBlockingPool() in https://codereview.chromium.org/250653002 to prevent people from accidentally forgetting to DCHECK the result, but this method is never DCHECK'ed anyway. So it seems that we can simply drop this attribute. ==========
Thanks for looking into this so quick! On 2016/01/07 19:57:24, Mostyn Bramley-Moore wrote: > Argh, patchset 1 hits an overenthusiastic windows toolchain warning in > plugin_service_impl.cc (visual studio doesn't count calling a static method via > a variable as referencing the variable). > > An alternative solution would be to drop WARN_UNUSED_RESULT from > BrowserThread::GetBlockingPool(). It looks like this was added in > https://codereview.chromium.org/250653002 to avoid forgetting to wrap calls to > some functions in DCHECK(), but I can't find any instances of > BrowserThread::GetBlockingPool() being DCHECK'ed. > > @Lei Zhang: does this look OK? I was a bit confused at first, because I didn't actually touch BrowserThread::GetBlockingPool(), but it turns out this is in fact not a GCC bug. SequencedWorkerPool::GetSequenceToken() is now a static method, so the return value of BrowserThread::GetBlockingPool() is indeed unused! Can we just change the call site in plugin_service_impl.cc to directly call SequencedWorkerPool::GetSequenceToken()?
On 2016/01/07 20:06:55, Bernhard Bauer wrote: > Thanks for looking into this so quick! > > On 2016/01/07 19:57:24, Mostyn Bramley-Moore wrote: > > Argh, patchset 1 hits an overenthusiastic windows toolchain warning in > > plugin_service_impl.cc (visual studio doesn't count calling a static method > via > > a variable as referencing the variable). > > > > An alternative solution would be to drop WARN_UNUSED_RESULT from > > BrowserThread::GetBlockingPool(). It looks like this was added in > > https://codereview.chromium.org/250653002 to avoid forgetting to wrap calls to > > some functions in DCHECK(), but I can't find any instances of > > BrowserThread::GetBlockingPool() being DCHECK'ed. > > > > @Lei Zhang: does this look OK? > > I was a bit confused at first, because I didn't actually touch > BrowserThread::GetBlockingPool(), but it turns out this is in fact not a GCC > bug. SequencedWorkerPool::GetSequenceToken() is now a static method, so the > return value of BrowserThread::GetBlockingPool() is indeed unused! Can we just > change the call site in plugin_service_impl.cc to directly call > SequencedWorkerPool::GetSequenceToken()? (And the other call sites as well, I mean.)
The CQ bit was checked by mostynb@opera.com to run a CQ dry run
> > I was a bit confused at first, because I didn't actually touch > > BrowserThread::GetBlockingPool(), but it turns out this is in fact not a GCC > > bug. SequencedWorkerPool::GetSequenceToken() is now a static method, so the > > return value of BrowserThread::GetBlockingPool() is indeed unused! Can we just > > change the call site in plugin_service_impl.cc to directly call > > SequencedWorkerPool::GetSequenceToken()? > > (And the other call sites as well, I mean.) Ahh, nice detective work. Done in patchset 3, let's see if it works.
Dry run: CQ is trying da patch. Follow status at https://chromium-cq-status.appspot.com/patch-status/1567983003/40001 View timeline at https://chromium-cq-status.appspot.com/patch-timeline/1567983003/40001
LGTM, thank you!
(But please update the description before landing.)
Description was changed from ========== drop useless warn_unused_result attribute from BrowserThread::GetBlockingPool() Followup to https://codereview.chromium.org/1414793009/. This is a workaround for an apparent GCC bug. BrowserThread::GetBlockingPool() is used in several places as part of a method call chain, and GCC emits a warning that the result is unused. -Werror promotes these warnings to errors. WARN_UNUSED_RESULT was added to BrowserThread::GetBlockingPool() in https://codereview.chromium.org/250653002 to prevent people from accidentally forgetting to DCHECK the result, but this method is never DCHECK'ed anyway. So it seems that we can simply drop this attribute. ========== to ========== call static SequencedWorkerPool::GetSequenceToken() method directly Followup to https://codereview.chromium.org/1414793009/ in order to avoid an arguably over-strict GCC warning. SequencedWorkerPool::GetSequenceToken() is now a static method, so we should call it directly instead of via BrowserThread::GetBlockingPool()->GetSequenceToken() - since the BrowserThread::GetBlockingPool() result is not actually used. ==========
Description was changed from ========== call static SequencedWorkerPool::GetSequenceToken() method directly Followup to https://codereview.chromium.org/1414793009/ in order to avoid an arguably over-strict GCC warning. SequencedWorkerPool::GetSequenceToken() is now a static method, so we should call it directly instead of via BrowserThread::GetBlockingPool()->GetSequenceToken() - since the BrowserThread::GetBlockingPool() result is not actually used. ========== to ========== call static SequencedWorkerPool::GetSequenceToken() method directly Followup to https://codereview.chromium.org/1414793009/ in order to avoid an arguably over-strict GCC warning. SequencedWorkerPool::GetSequenceToken() is now a static method, GCC emits a warning for BrowserThread::GetBlockingPool()->GetSequenceToken() since the return value of BrowserThread::GetBlockingPool() is not used. We should just all the static method directly. ==========
Description was changed from ========== call static SequencedWorkerPool::GetSequenceToken() method directly Followup to https://codereview.chromium.org/1414793009/ in order to avoid an arguably over-strict GCC warning. SequencedWorkerPool::GetSequenceToken() is now a static method, GCC emits a warning for BrowserThread::GetBlockingPool()->GetSequenceToken() since the return value of BrowserThread::GetBlockingPool() is not used. We should just all the static method directly. ========== to ========== call static SequencedWorkerPool::GetSequenceToken() method directly Followup to https://codereview.chromium.org/1414793009/ in order to avoid a GCC unused result warning. SequencedWorkerPool::GetSequenceToken() is now a static method, GCC emits a warning for BrowserThread::GetBlockingPool()->GetSequenceToken() since the return value of BrowserThread::GetBlockingPool() is not used. We should just all the static method directly. ==========
Done. Just waiting for a rubber-stamp from Jochen now.
lgtm
The CQ bit was unchecked by commit-bot@chromium.org
Dry run: This issue passed the CQ dry run.
lgtm
The CQ bit was checked by mostynb@opera.com
CQ is trying da patch. Follow status at https://chromium-cq-status.appspot.com/patch-status/1567983003/40001 View timeline at https://chromium-cq-status.appspot.com/patch-timeline/1567983003/40001
Message was sent while issue was closed.
Description was changed from ========== call static SequencedWorkerPool::GetSequenceToken() method directly Followup to https://codereview.chromium.org/1414793009/ in order to avoid a GCC unused result warning. SequencedWorkerPool::GetSequenceToken() is now a static method, GCC emits a warning for BrowserThread::GetBlockingPool()->GetSequenceToken() since the return value of BrowserThread::GetBlockingPool() is not used. We should just all the static method directly. ========== to ========== call static SequencedWorkerPool::GetSequenceToken() method directly Followup to https://codereview.chromium.org/1414793009/ in order to avoid a GCC unused result warning. SequencedWorkerPool::GetSequenceToken() is now a static method, GCC emits a warning for BrowserThread::GetBlockingPool()->GetSequenceToken() since the return value of BrowserThread::GetBlockingPool() is not used. We should just all the static method directly. ==========
Message was sent while issue was closed.
Committed patchset #3 (id:40001)
Message was sent while issue was closed.
Description was changed from ========== call static SequencedWorkerPool::GetSequenceToken() method directly Followup to https://codereview.chromium.org/1414793009/ in order to avoid a GCC unused result warning. SequencedWorkerPool::GetSequenceToken() is now a static method, GCC emits a warning for BrowserThread::GetBlockingPool()->GetSequenceToken() since the return value of BrowserThread::GetBlockingPool() is not used. We should just all the static method directly. ========== to ========== call static SequencedWorkerPool::GetSequenceToken() method directly Followup to https://codereview.chromium.org/1414793009/ in order to avoid a GCC unused result warning. SequencedWorkerPool::GetSequenceToken() is now a static method, GCC emits a warning for BrowserThread::GetBlockingPool()->GetSequenceToken() since the return value of BrowserThread::GetBlockingPool() is not used. We should just all the static method directly. Committed: https://crrev.com/c965fd6ce9d24bf6ed897266c771cc9c6f20c1ef Cr-Commit-Position: refs/heads/master@{#368326} ==========
Message was sent while issue was closed.
Patchset 3 (id:??) landed as https://crrev.com/c965fd6ce9d24bf6ed897266c771cc9c6f20c1ef Cr-Commit-Position: refs/heads/master@{#368326} |
|||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||
