On Thu, 17 Feb 2022 at 00:59, Jason Merrill <ja...@redhat.com> wrote:
>
> On 2/16/22 02:16, Zhao Wei Liew wrote:
> > On Wed Feb 16, 2022 at 4:06 AM +08, Jason Merrill wrote:
> >>> Ah, I see. I found it a bit odd that gcc-commit-mklog auto-generated a
> >>> subject with "c:",
> >>> but I just went with it as I didn't know any better. Unfortunately, I
> >>> can't change it now on the current thread.
> >>
> >> That came from this line in the testcase:
> >>
> >> > +/* PR c/25689 */
> >>
> >> The PR should be c++/25689. Also, sometimes the bugzilla component
> >> isn't the same as the area of the compiler you're changing; the latter
> >> is what you want in the patch subject, so that the right people know to
> >> review it.
> >
> > Oh, I see. Thanks for the explanation. I've fixed the line.
> >
> >>> Ah, I didn't notice that. Sorry about that! I'm kinda new to the whole
> >>> mailing list setup so there are some kinks I have to iron out.
> >>
> >> FWIW it's often easier to send the patch as an attachment.
> >
> > Alright, I'll send patches as attachments instead. I originally sent
> > them as text as it is easier to comment on them.
>
> It is a bit more of a hassle in this case because your mail sender
> doesn't mark the patch as text, but rather application/mbox or
> application/x-patch, so my mail reader for patch review (Thunderbird)
> doesn't display it inline. I tried sending myself a patch through the
> gmail web interface, and it used text/x-patch, which is OK; what are you
> using to send?
>
> Maybe renaming the file to .txt before sending would help?
Hmm, in the end I used gmail to send the patch, so I'm not sure why it was
marked that way. I'll test it out again before sending another patch.
> >>> +/* Test non-empty class */
> >>> +void f2(B b1, B b2)
> >>> +{
> >>> + if (b1 = 0); /* { dg-warning "suggest parentheses" } */
> >>> + if (b1 = 0.); /* { dg-warning "suggest parentheses" } */
> >>> + if (b1 = b2); /* { dg-warning "suggest parentheses" } */
> >>> + if (b1.operator= (0));
> >>> +
> >>> + /* Ideally, we wouldn't warn for non-empty classes using trivial
> >>> + operator= (below), but we currently do as it is a MODIFY_EXPR. */
> >>> + // if (b1.operator= (b2));
> >>
> >> You can avoid it by calling suppress_warning on that MODIFY_EXPR in
> >> build_over_call.
> >
> > Unfortunately, that also affects the warning for if (b1 = b2) just 5
> > lines above. Both expressions seem to generate the same tree structure.
>
> True, you would need to put the call to suppress_warning in build_new_op
> around where CALL_EXPR_OPERATOR_SYNTAX is set.
It seems like that would suppress the warning for the case of if (b1 = b2)
instead of
if (b1.operator= (b2)). Do you mean to add the call to suppress_warning
in build_method_call instead?
This is what I've tried so far:
1. Call suppress_warning (result, ...) in the trivial_fn_p block in
build_new_op,
right above the comment "There won't be a CALL_EXPR" (line 6699).
This suppresses the warning for if (b1 = b2) but not for if
(b1.operator= (b2)).
2. Call suppress_warning (result, ...) in build_method_call, right after
the call to
build_over_call (line 11141). This suppresses the warning for if
(b1.operator= (b2))
and not if (b1 = b2).
Based on this, I think the 2nd option might be what we want here? Please
correct me if I'm
wrong. I'm also unsure if there are issues that might arise with this
change.
