On Wed, May 15, 2013 at 6:50 PM, Cary Coutant <ccout...@google.com> wrote: >> gcc/ChangeLog: >> >> * tree-cfg.c (locus_descrim_hasher::hash): Only hash lineno. >> (locus_descrim_hasher::equal): Likewise. >> (next_discriminator_for_locus): Likewise. >> (assign_discriminator): Add return value. >> (make_edges): Assign more discriminator if necessary. >> (make_cond_expr_edges): Likewise. >> (make_goto_expr_edges): Likewise. >> >> gcc/testsuite/ChangeLog: >> >> * gcc.dg/debug/dwarf2/discriminator.c: New Test. > > Looks OK to me, but I can't approve patches for tree-cfg.c. > > Two comments: > > (1) In the C++ conversion, it looks like someone misspelled "discrim" > in "locus_descrim_hasher". > > (2) Is this worth fixing in trunk when we'll eventually switch to a > per-instruction discriminator?
> This patch fixes a common case where one line is distributed to 3 BBs, > but only 1 discriminator is assigned. As far as I can see the patch makes discriminators coarser (by hashing and comparing on LOCATION_LINE and not on the location). It also has the changes like - assign_discriminator (entry_locus, then_bb); + if (assign_discriminator (entry_locus, then_bb)) + assign_discriminator (entry_locus, bb); which is what the comment refers to? I think those changes are short-sighted because what happens if the 2nd assign_discriminator call has exactly the same issue? Especially with the make_goto_expr_edges change there may be multiple predecessors where I cannot see how the change is correct. Yes, the change changes something and thus may fix the testcase but it doesn't change things in a predictable way as far as I can see. So - the change to compare/hash LOCATION_LINE looks good to me, but the assign_discriminator change needs more explaining. Richard. > -cary
