This is the mail archive of the
gdb-patches@sourceware.org
mailing list for the GDB project.
Opinion about -Wtautological-compare
- From: Simon Marchi <simon dot marchi at ericsson dot com>
- To: GDB Patches <gdb-patches at sourceware dot org>
- Date: Fri, 27 Oct 2017 23:24:05 -0400
- Subject: Opinion about -Wtautological-compare
- Authentication-results: sourceware.org; auth=none
- Authentication-results: spf=none (sender IP is ) smtp.mailfrom=simon dot marchi at ericsson dot com;
- Spamdiagnosticmetadata: NSPM
- Spamdiagnosticoutput: 1:99
Hi all,
When building with Clang, I see warnings like these in multiple files:
/home/emaisin/src/binutils-gdb/gdb/arm-tdep.c:10013:13: error: comparison of 0 <= unsigned expression is always true [-Werror,-Wtautological-compare]
if (0 <= insn_op1 && 3 >= insn_op1)
~ ^ ~~~~~~~~
/home/emaisin/src/binutils-gdb/gdb/arm-tdep.c:11722:20: error: comparison of unsigned expression >= 0 is always true [-Werror,-Wtautological-compare]
else if (opB >= 0 && opB <= 2)
~~~ ^ ~
They are saying that because the variable is unsigned, the comparison
"variable >= 0" will always be true. What do you think we should do with
this?
1) Remove the comparisons and keep the warning:
- if (0 <= insn_op1 && 3 >= insn_op1)
+ if (3 >= insn_op1)
2) Leave the comparisons, but put them in comments and keep the warning:
- if (0 <= insn_op1 && 3 >= insn_op1)
+ if (/* 0 <= insn_op1 && */ 3 >= insn_op1)
3) Make a util function "in_inclusive_range" and use it (and keep the warning). Something like:
static bool
in_inclusive_range (unsigned int value, unsigned int low, unsigned int high)
{
return value >= low && value <= high;
}
and
- if (0 <= insn_op1 && 3 >= insn_op1)
+ if (in_inclusive_range (insn_op1, 0, 3))
4) Leave the code as-is and remove the warning (add -Wno-tautological-compare to the build).
Personally, I think the warning is useful and can reveal bugs, so I'd like to keep
it. I lean towards 2 or 3, because they help convey the idea that we check if the
value is within a range. If you are following with the architecture manual on the
side, it will probably show the same range (0-3) for those bits, so it helps if the
code does the same.
Simon