[llvm] 5cb4419 - [Target/PPC] Silence an unused variable warning. NFC.

[David Blaikie via llvm-commits]

On Mon, Jun 15, 2020 at 11:17 AM Davidino Italiano <ditaliano at apple.com> wrote:
>
>
>
> > On Jun 15, 2020, at 11:15 AM, David Blaikie <dblaikie at gmail.com> wrote:
> >
> > Could you include a description of the change (rather than the
> > motivation for the change) in the commit message? Helps with
> > post-commit review of changes like this. (& somewhat more important
> > what was changed rather than the compiler warning that motivated the
> > change)
> >
>
> I always found this to be the standard lingo.

Yeah, I've been trying to change that by repeatedly providing this
feedback & hopefully leading by example in my own commits.

> “Silence unused variable warning by using the variable” is a better description?
> Really don’t feel like is superimportant & don’t want to argue about it so if you have a better wording I’ll use it ;)

(void) cast to suppress -Wunused-variable in assert

compared to:

eliminate unused variable only used in an assert
(or "collapse/inline unused variable into an assert")

In the first I'd also include (beyond the first line of the commit
message) an explanation of why a (void) cast was used rather than the
collapsing, perhaps (if it's not obvious from the code - I mean, once
someone's reading beyond the first line of the commit, they can see
the code & such).

The latter title I'm probably not going to bother reviewing the code -
the former (or the original commit message) I'll go look at the code
to see how the warning was suppressed and if it was done per the
guidance & avoiding the (void) cast where practical.

> > Also - if "CE" is only used in the assert, please roll it into the
> > assertion rather than using (void) casts (see the last part of this
> > section: https://llvm.org/docs/CodingStandards.html#assert-liberally
> > ), ie:
> >
> > assert(isInt<34>(cast<MCConstantExpr>(RHS)->getValue()) && "Value must
> > fit in 34 bits");
>
> Doing it.

Thanks!

>
> —
> Davide