This is the mail archive of the
gcc-patches@gcc.gnu.org
mailing list for the GCC project.
Re: Implement -Wduplicated-branches (PR c/64279) (v3)
- From: Marek Polacek <polacek at redhat dot com>
- To: Jakub Jelinek <jakub at redhat dot com>
- Cc: Jason Merrill <jason at redhat dot com>, GCC Patches <gcc-patches at gcc dot gnu dot org>, Joseph Myers <joseph at codesourcery dot com>
- Date: Thu, 5 Jan 2017 16:39:40 +0100
- Subject: Re: Implement -Wduplicated-branches (PR c/64279) (v3)
- Authentication-results: sourceware.org; auth=none
- References: <20161019110707.GJ2576@redhat.com> <20161020102835.GM2576@redhat.com> <20161024141018.GD5939@redhat.com> <20161025135919.GJ5939@redhat.com> <CADzB+2nFxXywkHK5dVWKR9i9zKCwne6YWKdo531xynGFVpB6Rw@mail.gmail.com> <20161101135358.GP3541@tucnak.redhat.com> <20161103112447.GT5939@redhat.com> <CADzB+2nphvPrhK05o4zsWw-3zjB_Q+3NNqCdSYT0cxeuwC9XPA@mail.gmail.com> <20161103133850.GJ3541@tucnak.redhat.com>
Coming back to this...
On Thu, Nov 03, 2016 at 02:38:50PM +0100, Jakub Jelinek wrote:
> On Thu, Nov 03, 2016 at 09:27:55AM -0400, Jason Merrill wrote:
> > On Thu, Nov 3, 2016 at 7:24 AM, Marek Polacek <polacek@redhat.com> wrote:
> > > On Tue, Nov 01, 2016 at 02:53:58PM +0100, Jakub Jelinek wrote:
> > >> On Tue, Nov 01, 2016 at 09:41:20AM -0400, Jason Merrill wrote:
> > >> > On Tue, Oct 25, 2016 at 9:59 AM, Marek Polacek <polacek@redhat.com> wrote:
> > >> > > On Mon, Oct 24, 2016 at 04:10:21PM +0200, Marek Polacek wrote:
> > >> > >> On Thu, Oct 20, 2016 at 12:28:36PM +0200, Marek Polacek wrote:
> > >> > >> > I found a problem with this patch--we can't call do_warn_duplicated_branches in
> > >> > >> > build_conditional_expr, because that way the C++-specific codes might leak into
> > >> > >> > the hasher. Instead, I should use operand_equal_p, I think. Let me rework
> > >> > >> > that part of the patch.
> > >> >
> > >> > Hmm, is there a reason not to use operand_equal_p for
> > >> > do_warn_duplicated_branches as well? I'm concerned about hash
> > >> > collisions leading to false positives.
> > >>
> > >> If the hashing function is iterative_hash_expr / inchash::add_expr, then
> > >> that is supposed to pair together with operand_equal_p, we even have
> > >> checking verification of that.
> >
> > Yes, but there could still be hash collisions; we can't guarantee that
> > everything that compares unequal also hashes unequal.
>
> Right, after h0 == h1 is missing && operand_equal_p (thenb, elseb, 0)
> or so (the exact last operand needs to be figured out).
> OEP_ONLY_CONST is certainly wrong, we want the same VAR_DECLs to mean the
> same thing. 0 is a tiny bit better, but still it will give up on e.g. pure
> and other calls. OEP_PURE_SAME is tiny bit better than that, but still
> calls with the same arguments to the same function will not be considered
> equal, plus likely operand_equal_p doesn't handle STATEMENT_LIST etc.
> So maybe we need another OEP_* mode for this.
Yea, if I add "&& operand_equal_p (thenb, elseb, 0)" then this warning doesn't
trigger for certain cases, such as MODIFY_EXPR, RETURN_EXPR, probably
STATEMENT_LIST and others. So I suppose I could introduce a new OEP_ mode for
this (names? OEP_EXTENDED?) and then in operand_equal_p in case tcc_expression
do
case MODIFY_EXPR:
if (flags & OEP_EXTENDED)
// compare LHS and RHS of both
?
Marek