On Fri, Aug 09, 2024 at 11:03:24AM +1000, Nathaniel Shead wrote:
> On Thu, Aug 08, 2024 at 03:16:24PM -0400, Marek Polacek wrote:
> > On Thu, Aug 08, 2024 at 09:13:05AM +1000, Nathaniel Shead wrote:
> > > diff --git a/gcc/cp/error.cc b/gcc/cp/error.cc
> > > index 6c22ff55b46..03c19e4a7e4 100644
> > > --- a/gcc/cp/error.cc
> > > +++ b/gcc/cp/error.cc
> > > @@ -4782,12 +4782,14 @@ qualified_name_lookup_error (tree scope, tree
> > > name,
> > > scope);
> > > else if (TREE_CODE (decl) == TREE_LIST)
> > > {
> > > + auto_diagnostic_group d;
> > > error_at (location, "reference to %<%T::%D%> is ambiguous",
> > > scope, name);
> > > print_candidates (decl);
> > > }
> > > else
> > > {
> > > + auto_diagnostic_group d;
> > > name_hint hint;
> > > if (SCOPED_ENUM_P (scope) && TREE_CODE (name) == IDENTIFIER_NODE)
> > > hint = suggest_alternative_in_scoped_enum (name, scope);
> >
> > I don't see why we need the second a_d_d here.
> >
>
> The 'suggest_alternative_in_scoped_enum' call can register an 'inform'
> to be called in 'name_hint's destructor, specifically contained within
> name_hint::m_deferred (a deferred_diagnostic).
>
> Most other uses of 'name_hint' that I could find seemed to be correctly
> grouped already.
Ah okay.
> > > @@ -3534,6 +3536,7 @@ finish_class_member_access_expr (cp_expr object,
> > > tree name, bool template_p,
> > > else
> > > {
> > > /* Look up the member. */
> > > + auto_diagnostic_group d;
> > > access_failure_info afi;
> > > if (processing_template_decl)
> > > /* Even though this class member access expression is at this
> >
> > I don't quite see why we need it here, either.
> >
>
> A little later on in this block there's afi.maybe_suggest_accessor,
> which emits an 'inform' with a fixit suggesting how to access the
> member, which I feel should probably be part of the same group as the
> error emitted from the 'lookup_member' call.
Interesting.
> But I suppose this should be grouped more clearly, e.g.
>
> --- a/gcc/cp/typeck.cc
> +++ b/gcc/cp/typeck.cc
> @@ -3534,7 +3536,6 @@ finish_class_member_access_expr (cp_expr object, tree
> name, bool template_p,
> else
> {
> /* Look up the member. */
> - access_failure_info afi;
> if (processing_template_decl)
> /* Even though this class member access expression is at this
> point not dependent, the member itself may be dependent, and
> @@ -3543,12 +3544,18 @@ finish_class_member_access_expr (cp_expr object, tree
> name, bool template_p,
> ahead of time here; we're going to redo this member lookup at
> instantiation time anyway. */
> push_deferring_access_checks (dk_no_check);
> - member = lookup_member (access_path, name, /*protect=*/1,
> - /*want_type=*/false, complain,
> - &afi);
> - if (processing_template_decl)
> - pop_deferring_access_checks ();
> - afi.maybe_suggest_accessor (TYPE_READONLY (object_type));
> +
> + {
> + auto_diagnostic_group d;
> + access_failure_info afi;
> + member = lookup_member (access_path, name, /*protect=*/1,
> + /*want_type=*/false, complain,
> + &afi);
> + if (processing_template_decl)
> + pop_deferring_access_checks ();
> + afi.maybe_suggest_accessor (TYPE_READONLY (object_type));
> + }
> +
> if (member == NULL_TREE)
> {
> if (dependentish_scope_p (object_type))
>
> > > @@ -10384,6 +10401,8 @@ convert_for_assignment (tree type, tree rhs,
> > > {
> > > if (complain & tf_error)
> > > {
> > > + auto_diagnostic_group d;
> > > +
> > > /* If the right-hand side has unknown type, then it is an
> > > overloaded function. Call instantiate_type to get error
> > > messages. */
> > > @@ -10406,7 +10425,6 @@ convert_for_assignment (tree type, tree rhs,
> > > (rhs_loc,
> > > has_loc ? &label : NULL,
> > > has_loc ? highlight_colors::percent_h : NULL);
> > > - auto_diagnostic_group d;
> >
> > Oh I see, it was supposed to be in the outer block. OK.
> >
> > The patch looks good to me, thanks.
> >
> > Marek
> >
>
> Thanks, I'll wait for final approval from someone for once I've finished
> bootstrap+regtest with the above adjustment.
Nice. Thanks for cleaning this up!
Marek
