On Tue, Nov 11, 2014 at 8:26 AM, Jakub Jelinek <ja...@redhat.com> 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. > 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 see two ways out here. One is to add overloads to all the functions taking the special types like tree gimple_assign_rhs1 (gimple *); or simply add gassign *operator ()(gimple *g) { return as_a <gassign *> (g); } into a gimple-compat.h header which you include in places that are not converted "nicely". Both avoid manually making the compiler happy (which the explicit as_a<> stuff is! It doesn't add any "checking" - it's just placing the as_a<> at the callers and thus make the runtine ICE fire there). As much as I don't like "global" conversion operators I don't like adding overloads to all of the accessor functions even more. Whether you enable them generally or just for selected files via a gimple-compat.h will be up to you (but I'd rather get rid of them at some point). Note this allows seamless transform of "random" functions taking a gimple now but really only expecting a single kind. Note that we don't absolutely have to rush this all in for GCC 5. Being the very first for GCC 6 stage1 is another possibility. We just should get it right. Thanks, Richard. > Jakub