[llvm] c546825 - [llvm] Fix unused variable warnings

[David Blaikie via llvm-commits]

On Tue, Jun 9, 2020 at 2:33 AM Kadir Çetinkaya <kadircet at google.com> wrote:
>
> Hi David,
>
> I was doing that in the past, until someone else sent a message saying
that the general convention in LLVM is to rather put a `(void)x;`
statement. Unfortunately I can't find the mailing thread right now.

It'd be handy to have that for reference. (void)x is generally used if the
same expression is needed multiple times in one or more assertions, if the
value needs to be computed far from the assertion (eg: retrieve the size of
a container, then do some algorithm that changes that size, then assert
that the size before has some relation to the size after), or the value is
computed by something which has necessary side effects ("do the thing" then
"assert the thing succeeded" - you can't put the "do the thing" in the
assert, or a non-asserts build won't "do the thing").

> I would be more happy with inlining, as it *might* get rid of some
redundant calculations in release builds.

The other motivation is that the expression/variable won't stick around
accidentally if the assertion is removed or moved.

> Do we have any written consensus around this one to direct people in the
future for one way or another?

It's discussed a little towards the end of
https://llvm.org/docs/CodingStandards.html#assert-liberally

Another issue is that values used only by assertions will produce an
“unused value” warning when assertions are disabled. For example, this code
will warn:

unsigned Size = V.size();assert(Size > 42 && "Vector smaller than it
should be");
bool NewToSet = Myset.insert(Value);assert(NewToSet && "The value
shouldn't be in the set yet");

These are two interesting different cases. In the first case, the call to
V.size() is only useful for the assert, and we don’t want it executed when
assertions are disabled. Code like this should move the call into the
assert itself. In the second case, the side effects of the call must happen
whether the assert is enabled or not. In this case, the value should be
cast to void to disable the warning. To be specific, it is preferred to
write the code like this:

assert(V.size() > 42 && "Vector smaller than it should be");
bool NewToSet = Myset.insert(Value); (void)NewToSet;assert(NewToSet &&
"The value shouldn't be in the set yet");


- Dave

>
> On Tue, Jun 9, 2020 at 4:46 AM David Blaikie <dblaikie at gmail.com> wrote:
>>
>> If a variable is only used once in the assertion ,could you roll the
>> initialization into the assert (folding the named variable away)?
>>
>> On Wed, Jun 3, 2020 at 2:49 AM Kadir Cetinkaya via llvm-commits
>> <llvm-commits at lists.llvm.org> wrote:
>> >
>> >
>> > Author: Kadir Cetinkaya
>> > Date: 2020-06-03T11:49:01+02:00
>> > New Revision: c5468253aa555c2df98bd1f49d1e9d44b0150a2e
>> >
>> > URL:
https://github.com/llvm/llvm-project/commit/c5468253aa555c2df98bd1f49d1e9d44b0150a2e
>> > DIFF:
https://github.com/llvm/llvm-project/commit/c5468253aa555c2df98bd1f49d1e9d44b0150a2e.diff
>> >
>> > LOG: [llvm] Fix unused variable warnings
>> >
>> > Added:
>> >
>> >
>> > Modified:
>> >     llvm/lib/CodeGen/AsmPrinter/DwarfDebug.cpp
>> >
>> > Removed:
>> >
>> >
>> >
>> >
################################################################################
>> > diff  --git a/llvm/lib/CodeGen/AsmPrinter/DwarfDebug.cpp
b/llvm/lib/CodeGen/AsmPrinter/DwarfDebug.cpp
>> > index 67504f827d63..061330677680 100644
>> > --- a/llvm/lib/CodeGen/AsmPrinter/DwarfDebug.cpp
>> > +++ b/llvm/lib/CodeGen/AsmPrinter/DwarfDebug.cpp
>> > @@ -807,6 +807,7 @@ static void collectCallSiteParameters(const
MachineInstr *CallMI,
>> >      auto Suc = std::next(CallMI->getIterator());
>> >      // Only one-instruction delay slot is supported.
>> >      auto BundleEnd = llvm::getBundleEnd(CallMI->getIterator());
>> > +    (void)BundleEnd;
>> >      assert(std::next(Suc) == BundleEnd &&
>> >             "More than one instruction in call delay slot");
>> >      // Try to interpret value loaded by instruction.
>> > @@ -856,7 +857,9 @@ void DwarfDebug::constructCallSiteEntryDIEs(const
DISubprogram &SP,
>> >        return false;
>> >      auto Suc = std::next(MI.getIterator());
>> >      auto CallInstrBundle = getBundleStart(MI.getIterator());
>> > +    (void)CallInstrBundle;
>> >      auto DelaySlotBundle = getBundleStart(Suc);
>> > +    (void)DelaySlotBundle;
>> >      // Ensure that label after call is following delay slot
instruction.
>> >      // Ex. CALL_INSTRUCTION {
>> >      //       DELAY_SLOT_INSTRUCTION }
>> > @@ -1893,6 +1896,7 @@ void DwarfDebug::beginInstruction(const
MachineInstr *MI) {
>> >      if (!MI.isBundledWithSucc())
>> >        return false;
>> >      auto Suc = std::next(MI.getIterator());
>> > +    (void)Suc;
>> >      // Ensure that delay slot instruction is successor of the call
instruction.
>> >      // Ex. CALL_INSTRUCTION {
>> >      //        DELAY_SLOT_INSTRUCTION }
>> >
>> >
>> >
>> > _______________________________________________
>> > llvm-commits mailing list
>> > llvm-commits at lists.llvm.org
>> > https://lists.llvm.org/cgi-bin/mailman/listinfo/llvm-commits
-------------- next part --------------
An HTML attachment was scrubbed...
URL: <http://lists.llvm.org/pipermail/llvm-commits/attachments/20200609/931f77c3/attachment.html>