On Tue, 2014-11-11 at 08:26 +0100, Jakub Jelinek wrote: > On Mon, Nov 10, 2014 at 05:27:50PM -0500, David Malcolm wrote: > > On Sat, 2014-11-08 at 14:56 +0100, Jakub Jelinek wrote: > > > On Sat, Nov 08, 2014 at 01:07:28PM +0100, Richard Biener wrote: > > > > To be constructive here - the above case is from within a > > > > GIMPLE_ASSIGN case label > > > > and thus I'd have expected > > > > > > > > case GIMPLE_ASSIGN: > > > > { > > > > gassign *a1 = as_a <gassign *> (s1); > > > > gassign *a2 = as_a <gassign *> (s2); > > > > lhs1 = gimple_assign_lhs (a1); > > > > lhs2 = gimple_assign_lhs (a2); > > > > if (TREE_CODE (lhs1) != SSA_NAME > > > > && TREE_CODE (lhs2) != SSA_NAME) > > > > return (operand_equal_p (lhs1, lhs2, 0) > > > > && gimple_operand_equal_value_p (gimple_assign_rhs1 > > > > (a1), > > > > gimple_assign_rhs1 > > > > (a2))); > > > > else if (TREE_CODE (lhs1) == SSA_NAME > > > > && TREE_CODE (lhs2) == SSA_NAME) > > > > return vn_valueize (lhs1) == vn_valueize (lhs2); > > > > return false; > > > > } > > > > > > > > instead. That's the kind of changes I have expected and have approved > > > > of. > > > > > > But even that looks like just adding extra work for all developers, with > > > no > > > gain. You only have to add extra code and extra temporaries, in switches > > > typically also have to add {} because of the temporaries and thus extra > > > indentation level, and it doesn't simplify anything in the code. > > > > The branch attempts to use the C++ typesystem to capture information > > about the kinds of gimple statement we expect, both: > > (A) so that the compiler can detect type errors, and > > (B) as a comprehension aid to the human reader of the code > > > > The ideal here is when function params and struct field can be > > strengthened from "gimple" to a subclass ptr. This captures the > > knowledge that every use of a function or within a struct has a given > > gimple code. > > I just don't like all the as_a/is_a stuff enforced everywhere, > it means more typing, more temporaries, more indentation. > So, as I view it, instead of the checks being done cheaply (yes, I think > the gimple checking as we have right now is very cheap) under the > hood by the accessors (gimple_assign_{lhs,rhs1} etc.), those changes > put the burden on the developers, who has to check that manually through > the as_a/is_a stuff everywhere, more typing and uglier syntax. > I just don't see that as a step forward, instead a huge step backwards. > But perhaps I'm alone with this.
I agree with you w.r.t. my later patches. I like my initial set of 89 patches, but I overdid things with the attempt to convert all of the gimple_assign_ accessors. > Can you e.g. compare the size of - lines in your patchset combined, and > size of + lines in your patchset? As in, if your changes lead to less > typing or more. I don't know if this data matches the proposed interpretation (autoindentation in emacs is wonderful), but here goes: Diff of my current working copy vs last merge point, ignoring ChangeLog additions, obtaining lines starting "-", piping into wc: $ git diff fddbd0194b01f44c5b5f16379fd5405dcf6d71c0 $(git diff fddbd0194b01f44c5b5f16379fd5405dcf6d71c0 | diffstat -lp1 | grep -v ChangeLog) | grep -e "^-" | wc 6169 31032 272916 Likewise for lines starting with "+": $ git diff fddbd0194b01f44c5b5f16379fd5405dcf6d71c0 $(git diff fddbd0194b01f44c5b5f16379fd5405dcf6d71c0 | diffstat -lp1 | grep -v ChangeLog) | grep -e "^+" | wc 7295 36566 309380 so the + lines have 13% more bytes than the - lines Dave