Re: [PATCH] C++: Fix-it hints for '.' vs '->' (PR c++/84898)

[Jason Merrill <jason at redhat dot com>]

On Wed, Aug 29, 2018 at 4:37 PM, David Malcolm <dmalcolm@redhat.com> wrote:
> This patch implements fix-it hints for "." vs "->" mismatches in the C++
> frontend, for the places where the relevant location information is
> readily available.
>
> Successfully bootstrapped&regrtested on x86_64-pc-linux-gnu;
> adds 39 PASS results to g++.sum.
>
> OK for trunk?
>
> gcc/cp/ChangeLog:
>         PR c++/84898
>         * cp-tree.h (finish_class_member_access_expr): Add location_t
>         param.
>         (build_x_arrow): Likewise.
>         * parser.c (cp_parser_postfix_expression): Retain the location
>         of the "." or "->" token and pass it to
>         cp_parser_postfix_dot_deref_expression.
>         (cp_parser_postfix_dot_deref_expression): Add "loc_dot_or_deref"
>         param.  Pass it on to calls to build_x_arrow and
>         finish_class_member_access_expr.
>         (cp_parser_builtin_offsetof): Update for new param.
>         (cp_parser_range_for_member_function): Likewise.
>         (cp_parser_omp_var_list_no_open): Likewise.
>         * pt.c (tsubst_copy_and_build): Likewise.
>         * semantics.c (finish_id_expression): Likewise.
>         * typeck.c (finish_class_member_access_expr): Add new param
>         "loc_dot_or_deref".  When not UNKNOWN_LOCATION, use it to provide
>         a "->" fix-it hint for the pointer type error, and for the error's
>         location, rather than input_location, thus moving it from the
>         field name to the "." token.
>         * typeck2.c: Include "gcc-rich-location.h".
>         (build_x_arrow): Add param "loc_deref".  Use it to provide
>         a fix-it hint.
>
> gcc/objc/ChangeLog:
>         PR c++/84898
>         * objc-act.c (objc_build_component_ref): Update for new param.
>
> gcc/testsuite/ChangeLog:
>         PR c++/84898
>         * g++.dg/diagnostic/fixits.C: New test.
>         * g++.dg/parse/error20.C: Update expected column number
>         to be on the erroneous ".", rather than the fieldname.
>
> libcc1/ChangeLog:
>         PR c++/84898
>         * libcp1plugin.cc (plugin_build_binary_expr): Update for new
>         parameters of build_x_arrow and finish_class_member_access_expr.
> ---
>  gcc/cp/cp-tree.h                         |  4 +-
>  gcc/cp/parser.c                          | 39 +++++++++------
>  gcc/cp/pt.c                              |  4 +-
>  gcc/cp/semantics.c                       |  3 +-
>  gcc/cp/typeck.c                          | 20 ++++++--
>  gcc/cp/typeck2.c                         | 13 ++++-
>  gcc/objc/objc-act.c                      |  3 +-
>  gcc/testsuite/g++.dg/diagnostic/fixits.C | 83 ++++++++++++++++++++++++++++++++
>  gcc/testsuite/g++.dg/parse/error20.C     |  2 +-
>  libcc1/libcp1plugin.cc                   |  5 +-
>  10 files changed, 147 insertions(+), 29 deletions(-)
>  create mode 100644 gcc/testsuite/g++.dg/diagnostic/fixits.C
>
> diff --git a/gcc/cp/cp-tree.h b/gcc/cp/cp-tree.h
> index 055f2bc..56e99b2 100644
> --- a/gcc/cp/cp-tree.h
> +++ b/gcc/cp/cp-tree.h
> @@ -7244,7 +7244,7 @@ extern tree decay_conversion                      (tree,
>  extern tree build_class_member_access_expr      (cp_expr, tree, tree, bool,
>                                                  tsubst_flags_t);
>  extern tree finish_class_member_access_expr     (cp_expr, tree, bool,
> -                                                tsubst_flags_t);
> +                                                tsubst_flags_t, location_t);

Maybe give the new parameter a default argument of UNKNOWN_LOCATION
rather than add it to callers?

>  extern tree build_x_arrow                      (location_t, tree,
> -                                                tsubst_flags_t);
> +                                                tsubst_flags_t, location_t);
...
>  static tree cp_parser_postfix_dot_deref_expression
> -  (cp_parser *, enum cpp_ttype, cp_expr, bool, cp_id_kind *, location_t);
> +  (cp_parser *, enum cpp_ttype, cp_expr, bool, cp_id_kind *, location_t,
> +   location_t);

Do we really want to pass two different locations into these
functions?  Currently what we're passing is just the location of the
first token of the expression argument, so it seems redundant; perhaps
we could stick with one location parameter and pass the operator
location instead.

Jason