[committed] Trivial cleanups to new classes

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.

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.

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 *);
>  };
>
>

On 2017.11.02 at 08:55 -0600, Jeff Law 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.
> 
> In the end the arguments for dropping the "virtual" seemed stronger to me.
> 
> Bootstrapped and regression tested on x86.  Installing on the trunk.

Even specifying both override and final is normally frowned upon, see:
https://github.com/isocpp/CppCoreGuidelines/blob/master/CppCoreGuidelines.md#c128-virtual-functions-should-specify-exactly-one-of-virtual-override-or-final

On 11/02/2017 09:31 AM, Richard Biener wrote:
> 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
In the cases we're dealing with it has no semantic meaning.

jeff

On 11/02/2017 09:33 AM, Markus Trippelsdorf wrote:
> On 2017.11.02 at 08:55 -0600, Jeff Law 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.
>>
>> In the end the arguments for dropping the "virtual" seemed stronger to me.
>>
>> Bootstrapped and regression tested on x86.  Installing on the trunk.
> 
> Even specifying both override and final is normally frowned upon, see:
> https://github.com/isocpp/CppCoreGuidelines/blob/master/CppCoreGuidelines.md#c128-virtual-functions-should-specify-exactly-one-of-virtual-override-or-final
Yea, I hadn't researched that aspect as thoroughly.  ISTM we should 
probably pull some of this into our own guidelines.

Jeff