On Thu, Nov 2, 2017 at 3:55 PM, Jeff Law <l...@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 *);
> };
>
>
