On Thu, Nov 13, 2014 at 3:24 PM, Andrew MacLeod <amacl...@redhat.com> wrote: > On 11/13/2014 05:45 AM, Richard Biener wrote: >> >> On Thu, Nov 13, 2014 at 2:41 AM, David Malcolm <dmalc...@redhat.com> >> wrote: >>> >>> On Tue, 2014-11-11 at 11:43 +0100, Richard Biener wrote: >>>> >>>> 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". >>> >>> Thanks for the suggestions. >>> >>> Am I missing something, or is the gimple-compat.h idea above not valid C >>> ++? >>> >>> Note that "gimple" is still a typedef to >>> gimple_statement_base * >>> (as noted before, the gimple -> gimple * change would break everyone >>> else's patches, so we talked about that as a followup patch for early >>> stage3). >>> >>> Given that, if I try to create an "operator ()" outside of a class, I >>> get this error: >>> >>> ‘gassign* operator()(gimple)’ must be a nonstatic member function >>> >>> which is emitted from cp/decl.c's grok_op_properties: >>> /* An operator function must either be a non-static member >>> function >>> or have at least one parameter of a class, a reference to a >>> class, >>> an enumeration, or a reference to an enumeration. 13.4.0.6 */ >>> >>> I tried making it a member function of gimple_statement_base, but that >>> doesn't work either: we want a conversion >>> from a gimple_statement_base * to a gassign *, not >>> from a gimple_statement_base to a gassign *. >>> >>> Is there some syntactic trick here that I'm missing? Sorry if I'm being >>> dumb (I can imagine there's a way of doing it by making "gimple" become >>> some kind of wrapped ptr class, but that way lies madness, surely). >> >> Hmm. >> >> struct assign; >> struct base { >> operator assign *() const { return (assign *)this; } >> }; >> struct assign : base { >> }; >> >> void foo (assign *); >> void bar (base *b) >> { >> foo (b); >> } >> >> doesn't work, but >> >> void bar (base &b) >> { >> foo (b); >> } >> >> does. Indeed C++ doesn't seem to provide what is necessary >> for the compat trick :( >> >> So the gimple-compat.h header would need to provide >> additional overloads for the affected functions like >> >> inline tree >> gimple_assign_rhs1 (gimple *g) >> { >> return gimple_assign_rhs1 (as_a <gassign *> (g)); >> } >> >> that would work for me as well. >> >> > Won't that defeat the desire for checking tho? If you dont do a dyn_cast<> > in gimple_assign_rhs1 (gimple *g) anyone can call it and upcast any kind of > gimple into a gassign without checking that it is really a gassign... > Actually, a gcc_assert here may be sufficient... and better. > > It'd be pretty easy to miss someone calling it when its not a gassign.
as_a performs that checking. Richard. > Andrew > >