This is the mail archive of the gcc@gcc.gnu.org mailing list for the GCC project.
Index Nav: | [Date Index] [Subject Index] [Author Index] [Thread Index] | |
---|---|---|
Message Nav: | [Date Prev] [Date Next] | [Thread Prev] [Thread Next] |
Other format: | [Raw text] |
On 11/13/2014 05:45 AM, Richard Biener wrote:
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.On Thu, Nov 13, 2014 at 2:41 AM, David Malcolm <dmalcolm@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 <jakub@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.
It'd be pretty easy to miss someone calling it when its not a gassign. Andrew
Index Nav: | [Date Index] [Subject Index] [Author Index] [Thread Index] | |
---|---|---|
Message Nav: | [Date Prev] [Date Next] | [Thread Prev] [Thread Next] |