Re: [committed][PATCH] Convert sprintf warning code to a dominator walk

[Trevor Saunders <tbsaunde@tbsaunde.org>]

On Fri, Oct 27, 2017 at 02:03:59PM -0400, David Malcolm wrote:
> On Fri, 2017-10-27 at 10:55 -0600, Jeff Law wrote:
> > Prereq for eventually embedding range analysis into the sprintf
> > warning
> > pass.  The only thing that changed since the original from a few days
> > ago was the addition of FINAL OVERRIDE to the before_dom_children
> > override function.
> > 
> > Re-bootstrapped and regression tested on x86.
> > 
> > Installing on the trunk.  Final patch attached for archival purposes.
> > 
> > 
> > Jeff
> 
> Sorry to be re-treading the FINAL/OVERRIDE stuff, but...
> 
> [...snip...]
> 
> > +class sprintf_dom_walker : public dom_walker
> > +{
> > + public:
> > +  sprintf_dom_walker () : dom_walker (CDI_DOMINATORS) {}
> > +  ~sprintf_dom_walker () {}
> > +
> > +  virtual edge before_dom_children (basic_block) FINAL OVERRIDE;
> 
> Is it just me, or is it a code smell to have both "virtual" and
> "final"/"override" on a decl?

fwiw I'm of the opinion that all decls of a virtual function should be
marked as virtual including ones also marked as final or override.

> In particular, AIUI:
> "virtual" says: "some subclass might override this method"

I'd say virtual means that the function is generally called through a
vtable not directly, and that it may be overriding a super class method
and / or allow subclasses to override this function.

> "final" says: "no subclass will override this method"
> 
> so having both seems contradictory.

I guess that depends on how you interpret virtual.

> If sprintf_dom_walker is providing a implementation of a vfunc of
> dom_walker, then presumably this should just lose the "virtual" on the
> subclass, it's presumably already got the "virtual" it needs in the
> base class.

Its certainly not necessary, but I think its good to be explicit.
Especially if you mark the class as final instead of individual methods
it isn't obvious the method gets called indirectly without checking
the parent classes for declarations.

Trev

> 
> 
> Dave