This is the mail archive of the gcc-patches@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]

Re: [committed][PATCH] Trivial cleanups to new classes


On Thu, Nov 2, 2017 at 3:55 PM, Jeff Law <law@redhat.com> wrote:
>
> As has been discussed on-list.  This patch adds a virtual destructor to the
> new classes in tree-ssa-propagate.h per our coding conventions and what are
> considered best practices.  It doesn't matter for any code I'm aware of
> today -- it's a defensive measure.
>
> This also drops the "virtual" keyword on the FINAL OVERRIDE member functions
> in gimple-ssa-sprintf's sprintf_dom_walker class.  Opinions here are more
> mixed.  It's agreed that the keyword is redundant in this context.  The
> question is whether or not it adds confusion or reduces confusion.
>
> The virtual keyword intuitively implies to me the member can be overridden
> by a derived class, but that's in direct conflict with the FINAL keyword.
>
> Others focus more on the fact that the virtual keyword implies that the
> calls are typically indirect.   But in the case of a FINAL, one of the hopes
> is that devirt can use the information to change the indirect call into a
> direct call.

Does omitting 'virtual' have semantic meaning in C++?  I don't see
code-generation
differences for

struct X {
    virtual void foo (void);
    void bar();
};
struct Y : public X {
    void foo (void);
    void baz();
};

void X::bar()
{
  foo ();
}

void Y::baz()
{
  foo ();
}

when looking at bar vs. baz.  Even deriving from Y and overriding foo is
valid again.

Richard.

> In the end the arguments for dropping the "virtual" seemed stronger to me.
>
> Bootstrapped and regression tested on x86.  Installing on the trunk.
>
> Jeff
>
> ps. I suspect there's similar cleanups we ought to be doing on other classes
> used within GCC.
>
>
>         * gimple-ssa-sprintf.c (sprintf_dom_walker): Remove
>         virtual keyword on FINAL OVERRIDE members.
>
>         * tree-ssa-propagate.h (ssa_propagation_engine): Group
>         virtuals together.  Add virtual destructor.
>         (substitute_and_fold_engine): Similarly.
>
> diff --git a/gcc/gimple-ssa-sprintf.c b/gcc/gimple-ssa-sprintf.c
> index 7415413..35ceb2c 100644
> --- a/gcc/gimple-ssa-sprintf.c
> +++ b/gcc/gimple-ssa-sprintf.c
> @@ -120,7 +120,7 @@ class sprintf_dom_walker : public dom_walker
>    sprintf_dom_walker () : dom_walker (CDI_DOMINATORS) {}
>    ~sprintf_dom_walker () {}
>
> -  virtual edge before_dom_children (basic_block) FINAL OVERRIDE;
> +  edge before_dom_children (basic_block) FINAL OVERRIDE;
>    bool handle_gimple_call (gimple_stmt_iterator *);
>
>    struct call_info;
> diff --git a/gcc/tree-ssa-propagate.h b/gcc/tree-ssa-propagate.h
> index 629ae77..be4500b 100644
> --- a/gcc/tree-ssa-propagate.h
> +++ b/gcc/tree-ssa-propagate.h
> @@ -81,14 +81,16 @@ class ssa_propagation_engine
>  {
>   public:
>
> -  /* Main interface into the propagation engine.  */
> -  void ssa_propagate (void);
> +  virtual ~ssa_propagation_engine (void) { }
>
>    /* Virtual functions the clients must provide to visit statements
>       and phi nodes respectively.  */
>    virtual enum ssa_prop_result visit_stmt (gimple *, edge *, tree *) = 0;
>    virtual enum ssa_prop_result visit_phi (gphi *) = 0;
>
> +  /* Main interface into the propagation engine.  */
> +  void ssa_propagate (void);
> +
>   private:
>    /* Internal implementation details.  */
>    void simulate_stmt (gimple *stmt);
> @@ -100,10 +102,12 @@ class ssa_propagation_engine
>  class substitute_and_fold_engine
>  {
>   public:
> -  bool substitute_and_fold (void);
> -  bool replace_uses_in (gimple *);
> +  virtual ~substitute_and_fold_engine (void) { }
>    virtual bool fold_stmt (gimple_stmt_iterator *) { return false; }
>    virtual tree get_value (tree) { return NULL_TREE; }
> +
> +  bool substitute_and_fold (void);
> +  bool replace_uses_in (gimple *);
>    bool replace_phi_args_in (gphi *);
>  };
>
>


Index Nav: [Date Index] [Subject Index] [Author Index] [Thread Index]
Message Nav: [Date Prev] [Date Next] [Thread Prev] [Thread Next]