Modify ALLOW_UNUSED to allow enabling unused local warnings on MSVC.

Modify ALLOW_UNUSED to allow enabling unused local warnings on MSVC.

This splits the macro into two:

int a = 1;
ALLOW_UNUSED_LOCAL(a);

and

typedef Foo Bar ALLOW_UNUSED_TYPE;
void foo() ALLOW_UNUSED_TYPE;  // ALLOW_UNUSED_TYPE_OR_FUNC seemed too verbose

This matches changes that have already been made in Blink.

BUG=81439
TEST=none
TBR=ben

Committed: https://crrev.com/99867ef78bf14ad37e263860248d0a2d61b51a73
Cr-Commit-Position: refs/heads/master@{#300014}

[Peter Kasting, 2014-10-14 00:15:36]

pkasting@chromium.org changed reviewers:
+ ben@chromium.org

[Peter Kasting, 2014-10-16 01:03:46]

ping beng

[danakj, 2014-10-16 15:27:05]

danakj@chromium.org changed reviewers:
+ danakj@chromium.org

[danakj, 2014-10-16 15:27:06]

LGTM

https://codereview.chromium.org/650393002/diff/20001/base/logging_unittest.cc
File base/logging_unittest.cc (right):

https://codereview.chromium.org/650393002/diff/20001/base/logging_unittest.cc...
base/logging_unittest.cc:243: ALLOW_UNUSED_TYPE std::ostream&
operator<<(std::ostream& out,
nit: can you put this ALLOW on the other side of the function arguments? maybe
that will make it read less like it's talking about the return type.

[danakj, 2014-10-16 15:27:53]

On 2014/10/16 15:27:06, danakj wrote:
> LGTM

(for base/ and cc/)

[Peter Kasting, 2014-10-16 17:39:06]

Moving Ben to TBR for the remaining couple of files.

https://codereview.chromium.org/650393002/diff/20001/base/logging_unittest.cc
File base/logging_unittest.cc (right):

https://codereview.chromium.org/650393002/diff/20001/base/logging_unittest.cc...
base/logging_unittest.cc:243: ALLOW_UNUSED_TYPE std::ostream&
operator<<(std::ostream& out,
On 2014/10/16 15:27:06, danakj wrote:
> nit: can you put this ALLOW on the other side of the function arguments? maybe
> that will make it read less like it's talking about the return type.

Done.

[Peter Kasting, 2014-10-16 17:39:10]

The CQ bit was checked by pkasting@chromium.org

[commit-bot: I haz the power, 2014-10-16 17:40:13]

CQ is trying da patch. Follow status at
 https://chromium-cq-status.appspot.com/patch-status/650393002/40001

[commit-bot: I haz the power, 2014-10-16 17:53:23]

The CQ bit was unchecked by commit-bot@chromium.org

[commit-bot: I haz the power, 2014-10-16 17:53:25]

Try jobs failed on following builders:
  android_chromium_gn_compile_dbg on tryserver.chromium.linux
(http://build.chromium.org/p/tryserver.chromium.linux/builders/android_chromiu...)

[Peter Kasting, 2014-10-16 19:37:42]

The CQ bit was checked by pkasting@chromium.org

[commit-bot: I haz the power, 2014-10-16 19:39:21]

CQ is trying da patch. Follow status at
 https://chromium-cq-status.appspot.com/patch-status/650393002/60001

[commit-bot: I haz the power, 2014-10-16 19:53:01]

The CQ bit was unchecked by commit-bot@chromium.org

[commit-bot: I haz the power, 2014-10-16 19:53:03]

Try jobs failed on following builders:
  android_chromium_gn_compile_dbg on tryserver.chromium.linux
(http://build.chromium.org/p/tryserver.chromium.linux/builders/android_chromiu...)

[Peter Kasting, 2014-10-16 20:06:26]

So apparently I can't move the ALLOW_UNUSED_TYPE in logging_unittest.cc, or
remove it, because both cause compile bustage.  Reverting that.

[danakj, 2014-10-16 20:08:52]

On 2014/10/16 20:06:26, Peter Kasting wrote:
> So apparently I can't move the ALLOW_UNUSED_TYPE in logging_unittest.cc, or
> remove it, because both cause compile bustage.  Reverting that.

Oh, darn. GCC docs say it can be there. Maybe clang disagrees.

[Peter Kasting, 2014-10-16 20:10:44]

The CQ bit was checked by pkasting@chromium.org

[commit-bot: I haz the power, 2014-10-16 20:12:07]

CQ is trying da patch. Follow status at
 https://chromium-cq-status.appspot.com/patch-status/650393002/80001

[commit-bot: I haz the power, 2014-10-16 23:49:46]

Committed patchset #5 (id:80001)

[commit-bot: I haz the power, 2014-10-16 23:50:47]

Patchset 5 (id:??) landed as
https://crrev.com/99867ef78bf14ad37e263860248d0a2d61b51a73
Cr-Commit-Position: refs/heads/master@{#300014}

[Nico, 2014-10-25 22:56:07]

thakis@chromium.org changed reviewers:
+ thakis@chromium.org

[Nico, 2014-10-25 22:56:08]

https://codereview.chromium.org/650393002/diff/80001/base/compiler_specific.h
File base/compiler_specific.h (right):

https://codereview.chromium.org/650393002/diff/80001/base/compiler_specific.h...
base/compiler_specific.h:99: #define ALLOW_UNUSED_LOCAL(x) false ? (void)x :
(void)0
How is this different from ignore_result()?

Why not just (void)x, why the `false?:`?

[Peter Kasting, 2014-10-27 17:58:44]

https://codereview.chromium.org/650393002/diff/80001/base/compiler_specific.h
File base/compiler_specific.h (right):

https://codereview.chromium.org/650393002/diff/80001/base/compiler_specific.h...
base/compiler_specific.h:99: #define ALLOW_UNUSED_LOCAL(x) false ? (void)x :
(void)0
On 2014/10/25 22:56:08, Nico wrote:
> How is this different from ignore_result()?

It doesn't implicitly take the address of |x| or in any other way evaluate it
(see below).

It would also be strange for code to write "ignore_result(x)" as a way of
marking |x| as "allowed to be unused" -- that could be very confusing to a
reader depending on the situation.

> Why not just (void)x, why the `false?:`?

This references |x| without any possibility of actually evaluating it, just in
case evaluating the argument has some effect (e.g. x is declared volatile and so
we generate a load the compiler can't optimize away, or I suppose maybe this
gets used inside another macro and |x| turns out to be some expression with side
effects).

I think it's unlikely the distinction actually matters, but this form is at
least as safe as the other, so I don't see anything wrong with using it.

[Nico, 2014-10-27 18:52:32]

Makes sense, thanks.

On Mon, Oct 27, 2014 at 10:58 AM, <pkasting@chromium.org> wrote:

>
> https://codereview.chromium.org/650393002/diff/80001/base/
> compiler_specific.h
> File base/compiler_specific.h (right):
>
> https://codereview.chromium.org/650393002/diff/80001/base/
> compiler_specific.h#newcode99
> base/compiler_specific.h:99: #define ALLOW_UNUSED_LOCAL(x) false ?
> (void)x : (void)0
> On 2014/10/25 22:56:08, Nico wrote:
>
>> How is this different from ignore_result()?
>>
>
> It doesn't implicitly take the address of |x| or in any other way
> evaluate it (see below).
>
> It would also be strange for code to write "ignore_result(x)" as a way
> of marking |x| as "allowed to be unused" -- that could be very confusing
> to a reader depending on the situation.
>
>  Why not just (void)x, why the `false?:`?
>>
>
> This references |x| without any possibility of actually evaluating it,
> just in case evaluating the argument has some effect (e.g. x is declared
> volatile and so we generate a load the compiler can't optimize away, or
> I suppose maybe this gets used inside another macro and |x| turns out to
> be some expression with side effects).
>
> I think it's unlikely the distinction actually matters, but this form is
> at least as safe as the other, so I don't see anything wrong with using
> it.
>
> https://codereview.chromium.org/650393002/
>

To unsubscribe from this group and stop receiving emails from it, send an email
to chromium-reviews+unsubscribe@chromium.org.