[llvm] r179669 - Fix -Werror build.

[Jack Carter]

Rafael,

I am not saying he is not on the list. I just want to make sure he sees it.

Jack
________________________________________
From: Rafael Ávila De Espíndola [rafael.espindola at gmail.com]
Sent: Thursday, April 18, 2013 5:45 PM
To: Jack Carter
Cc: David Blaikie; Evgeniy Stepanov; Vladimir Medic; llvm-commits at cs.uiuc.edu
Subject: Re: [llvm] r179669 - Fix -Werror build.

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