For reference/discussion.
GCC complains about signed-vs-unsigned comparison. I'm actually surprised that m_registers_count is a signed integer, as I can hardly imagine a negative register count. I'm under the impression that we could change m_register_count fields to be unsigned, but that would be a larger change. Thoughts?
Details
Details
Diff Detail
Diff Detail
- Repository
- rL LLVM
Event Timeline
Comment Actions
I am confused. m_registers_count is declared as uint8_t at RegisterContextPOSIX_mips64.h:67...
Comment Actions
I'm getting more convinced the warning emitted by GCC is bogus
/home/davide/work/llvm-lldb/tools/lldb/source/Plugins/Process/Utility/RegisterContextPOSIX_mips64.cpp:58:26: warning: comparison between signed and unsigned integer expressions [-Wsign-compare] assert(m_num_registers == m_registers_count[gpr_registers_count] + ~~~~~~~~~~~~~~~~^~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~ m_registers_count[fpr_registers_count] + ~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~ m_registers_count[msa_registers_count]); ~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~
Comment Actions
And I was wrong. @nlewycky explained on IRC:
14:23 < nlewycky> gcc is correct, in 'char + char' each char gets promoted to 'int' first then summed, then you've got an unsigned int == int comparison 14:23 < nlewycky> uint8_t and unsigned char are also promoted to int (not unsigned int) before the addition
Comment Actions
Reference for the future (http://www.open-std.org/jtc1/sc22/open/n2356/conv.html [conv.prom1])
source/Plugins/Process/Utility/RegisterContextPOSIX_mips64.cpp | ||
---|---|---|
59 ↗ | (On Diff #95485) | If you're gonna go with casting, I think using the same type as m_num_registers makes more sense. assert(m_num_registers == static_cast<uint32_t>(...)); |
Comment Actions
Thanks for digging into this, I've learned something new today. Fixing this with a cast seems reasonable then.