[PATCH] D27337: [globalisel][aarch64] Fix unintended assumptions about PartialMappingIdx. NFC.

[Quentin Colombet via llvm-commits]

> On Dec 5, 2016, at 9:22 AM, Quentin Colombet via llvm-commits <llvm-commits at lists.llvm.org> wrote:
> 
>> 
>> On Dec 5, 2016, at 3:54 AM, Daniel Sanders via Phabricator <reviews at reviews.llvm.org> wrote:
>> 
>> dsanders added inline comments.
>> 
>> 
>> ================
>> Comment at: lib/Target/AArch64/AArch64RegisterBankInfo.cpp:118
>>    const PartialMapping &Map =                                                \
>> -        AArch64::PartMappings[AArch64::PartialMappingIdx::Idx];                \
>> +        AArch64::PartMappings[AArch64::Idx - AArch64::PartialMappingIdx_Min];  \
>>    (void) Map;                                                                \
>> ----------------
>> qcolombet wrote:
>>> I'd keep `::PartialMappingIdx::`, I like being explicit about the type we use (and that shouldn't be redundant for the _Min part when you'd use a shorter name, see the other review).
>> Ok. Would you like me to make the explicit namespace mandatory by changing the type to an 'enum class'? This would also fix the prefix issue I mentioned on the other review.
> 
> No, following the prefix rule like you did in the other review is enough?

Scratch the question mark x)

> 
>> 
>> 
>> ================
>> Comment at: lib/Target/AArch64/AArch64RegisterBankInfo.cpp:135
>>  do {                                                                         \
>> -    AArch64::PartialMappingIdx PartialMapBaseIdx =                             \
>> -        AArch64::PartialMappingIdx::RBName##Size;                              \
>> +    unsigned PartialMapBaseIdx =                                               \
>> +        AArch64::RBName##Size - AArch64::PartialMappingIdx_Min;                \
>> ----------------
>> qcolombet wrote:
>>> Could we keep the PartialMappingIdx type instead of unsigned?
>> The compiler warns about using PartialMappingIdx because it's not a PartialMappingIdx after the subtraction. The warning could be silenced with a cast like so:
>> PartialMappingIdx PartialMapBaseIdx = (PartialMappingIdx)(AArch64::PartialMappingIdx::RBName##Size - AArch64::PartialMappingIdx::PartialMappingIdx_Min);
>> but this is still misleading because the values don't match up with the enumerators.
> 
> What to you mean they do not match up with the enumerators?
> The value should be within the enumerators at the end of the subtraction.
> 
>> 
>> 
>> https://reviews.llvm.org/D27337 <https://reviews.llvm.org/D27337>
>> 
>> 
>> 
> 
> _______________________________________________
> llvm-commits mailing list
> llvm-commits at lists.llvm.org <mailto:llvm-commits at lists.llvm.org>
> http://lists.llvm.org/cgi-bin/mailman/listinfo/llvm-commits <http://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/20161205/1372f72b/attachment.html>