On 12/16/2015 11:04 AM, David Malcolm wrote:
Currently trunk emits range information for most bad binary operations
in the C++ frontend; but not in the C frontend.
The helper function binary_op_error shared by C and C++ takes a
location_t. In the C++ frontend, a location_t containing the range
has already been built, so we get the underline when it later validates
the expression: e.g. this example from:
https://gcc.gnu.org/wiki/ClangDiagnosticsComparison
t.cc:9:19: error: no match for ‘operator+’ (operand types are ‘int’ and ‘foo’)
return P->bar() + *P;
~~~~~~~~~^~~~
In the C frontend, the "full" location_t is only built
after type-checking, so if type-checking fails, we get a caret/range
covering just the operator so e.g. for:
return (some_function ()
+ some_other_function ());
we get just:
gcc.dg/bad-binary-ops.c:29:4: error: invalid operands to binary + (have 'struct
s' and 'struct t')
+ some_other_function ());
^
The following patch updates binary_op_error to accept a rich_location *.
For the C++ frontend we populate it with just the location_t as before,
but for the C frontend we can add locations for the operands, giving
this underlining for the example above:
bad-binary-ops.c:29:4: error: invalid operands to binary + (have 'struct s' and
'struct t')
return (some_function ()
~~~~~~~~~~~~~~~~
+ some_other_function ());
^ ~~~~~~~~~~~~~~~~~~~~~~
Additionally, in the C++ frontend, cp_build_binary_op has an "error"
call, which implicitly uses input_location, giving this reduced
information for another test case from
https://gcc.gnu.org/wiki/ClangDiagnosticsComparison:
bad-binary-ops.C:10:14: error: invalid operands of types '__m128 {aka float}'
and 'const int*' to binary 'operator/'
myvec[1] / ptr;
^~~
The patch updates it to use error_at with the location_t provided,
fixing the above to become:
bad-binary-ops.C:10:12: error: invalid operands of types '__m128 {aka float}'
and 'const int*' to binary 'operator/'
myvec[1] / ptr;
~~~~~~~~~^~~~~
Finally, since this patch adds a usage of
gcc_rich_location::maybe_add_expr to cc1, we can drop the hacked-in
copy of gcc_rich_location::add_expr from
gcc.dg/plugin/diagnostic_plugin_show_trees.c.
Successfully bootstrapped®rtested on x86_64-pc-linux-gnu;
adds 15 new PASS results in g++.sum and 7 new PASS results in gcc.sum
OK for trunk in stage 3?
gcc/c-family/ChangeLog:
* c-common.c (binary_op_error): Convert first param from
location_t to rich_location * and use it when emitting an error.
* c-common.h (binary_op_error): Convert first param from
location_t to rich_location *.
gcc/c/ChangeLog:
* c-typeck.c: Include "gcc-rich-location.h".
(build_binary_op): In the two places that call binary_op_error,
create a gcc_rich_location and populate it with the location of
the binary op and its two operands.
gcc/cp/ChangeLog:
* typeck.c (cp_build_binary_op): Update for change in signature
of build_binary_op. Use error_at to replace an implicit use
of input_location with param "location" in "invalid operands"
error.
(cp_build_binary_op): Replace an error with an error_at, using
"location", rather than implicitly using input_location.
gcc/testsuite/ChangeLog:
* g++.dg/diagnostic/bad-binary-ops.C: New test case.
* gcc.dg/bad-binary-ops.c: New test case.
gcc.dg/plugin/diagnostic_plugin_show_trees.c (get_range_for_expr):
Remove material copied from gcc-rich-location.c
(gcc_rich_location::add_expr): Likewise.
So I'm of a mixed mind here.
We're well into stage3 and I can't see a reasonable way to call this a
bugfix, but I see the value here at the end user level and it looks to
be quite safe.
I'll tentatively approve -- give other maintainers 48hrs to object on
the grounds this isn't a bugfix before committing.
Jeff
