[llvm] r179669 - Fix -Werror build.

[Rafael Ávila De Espíndola]

Why is he not on the list? We do code reviews over email, so all developers should be subscribed.

Sent from my iPhone

On 2013-04-18, at 15:01, Jack Carter <Jack.Carter at imgtec.com> wrote:

> David,
> 
> Thanks for the feedback. I will forward this to code author (Vladimir).
> 
> Jack
> ________________________________________
> From: David Blaikie [dblaikie at gmail.com]
> Sent: Wednesday, April 17, 2013 10:28 PM
> To: Evgeniy Stepanov; Jack Carter
> Cc: llvm-commits at cs.uiuc.edu
> Subject: Re: [llvm] r179669 - Fix -Werror build.
> 
> On Tue, Apr 16, 2013 at 11:45 PM, Evgeniy Stepanov
> <eugeni.stepanov at gmail.com> wrote:
>> Author: eugenis
>> Date: Wed Apr 17 01:45:11 2013
>> New Revision: 179669
>> 
>> URL: http://llvm.org/viewvc/llvm-project?rev=179669&view=rev
>> Log:
>> Fix -Werror build.
>> 
>> Broken in r179657.
>> 
>> Modified:
>>    llvm/trunk/lib/Target/Mips/AsmParser/MipsAsmParser.cpp
>> 
>> Modified: llvm/trunk/lib/Target/Mips/AsmParser/MipsAsmParser.cpp
>> URL: http://llvm.org/viewvc/llvm-project/llvm/trunk/lib/Target/Mips/AsmParser/MipsAsmParser.cpp?rev=179669&r1=179668&r2=179669&view=diff
>> ==============================================================================
>> --- llvm/trunk/lib/Target/Mips/AsmParser/MipsAsmParser.cpp (original)
>> +++ llvm/trunk/lib/Target/Mips/AsmParser/MipsAsmParser.cpp Wed Apr 17 01:45:11 2013
>> @@ -1022,6 +1022,8 @@ const MCExpr* MipsAsmParser::evaluateRel
>>       if (LoSign)
>>         Val++;
>>       Res = MCConstantExpr::Create(Val, getContext());
>> +    } else {
>> +      llvm_unreachable("Invalid RelocStr value");
> 
> Jack - please check if this is the right fix, semantically. If it is,
> it should be changed to an assert, rather than unreachable.
> 
> ie, rather than this:
> if (X) {
> ...
> } else if (Y) {
> ...
> } else {
>  unreachable
> }
> 
> this:
> 
> if (X) {
>  ...
> } else {
>  assert (Y)
>  ...
> }
> 
> Also, in general code review feedback, it looks like "Res" could be
> removed, or reduced in scope - it's just used for one line of liveness
> in all the returns, in which case it could just s/Res = /return /
> 
>>     }
>>    return Res;
>>   }
>> 
>> 
>> _______________________________________________
>> llvm-commits mailing list
>> llvm-commits at cs.uiuc.edu
>> http://lists.cs.uiuc.edu/mailman/listinfo/llvm-commits
> 
> 
> _______________________________________________
> llvm-commits mailing list
> llvm-commits at cs.uiuc.edu
> http://lists.cs.uiuc.edu/mailman/listinfo/llvm-commits