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 <pola...@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 <pola...@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