[LLVMdev] Final added to parser<bool>

[David Blaikie]

On Thu, Mar 19, 2015 at 4:30 AM, Reed Kotler <Reed.Kotler at imgtec.com> wrote:

> One could argue that mclinker is doing something good or not by how it's
> using this class
> but I don't see the need for parser<bool> to be final. That is a
> subjective opinion that mclinker needs to
> be changed.
>
> I think that "final" was added  to some of these command line classes to
> avoid some kind of clang warning.
>
> That seems wrong to me that the tools are dictating something in an API.
>

Well the warning's a pretty good one - and dictates a way to design an API
safely to avoid the possibility of a bad delete.

It's not uncommon to have a polymorphic hierarchy that doesn't require
polymorphic ownership. To ensure that polymorphic ownership doesn't sneak
in, we have a warning to ensure the class either has an interface
compatible with polymorphic ownership (has a virtual dtor) or disallows
polymorphic ownership (has a protected dtor in any non-final base).

There is a narrower warning that only diagnoses this at the use of
polymorphic destruction (-Wdelete-non-virtual-dtor) but we've not needed to
fallback to that in LLVM so far & I'd mildly rather avoid doing so. The
protected-final-or-virtual pattern seems fairly elegant/self-documenting to
me.


> There may be other valid reasons to create derived classes.
>
> I think we should remove the "final" attribute on those classes and fix
> the clang warning issue in some
> better way than changing the API.
>
> If mclinker had been part of the llvm project tools tree that would not
> have been allowed even.
>

Or we might've decided that the use in mclinker was bad & should be fixed -
I haven't really looked at it to consider the details of its use, to be
honest.


>
> ________________________________________
> From: Simon Atanasyan [simon at atanasyan.com]
> Sent: Wednesday, March 18, 2015 9:35 PM
> To: Reed Kotler
> Cc: David Blaikie; Simon Atanasyan; Rich Fuhler; llvmdev at cs.uiuc.edu
> Subject: Re: [LLVMdev] Final added to parser<bool>
>
> Hi Reed,
>
> The FalseParser has a rather strange design. It's purpose to parse
> options like -no-to-do-something and always return false even if a
> user provides -no-to-do-something=no. That should be fixed on the
> MCLinker side.
>
> On Thu, Mar 19, 2015 at 5:14 AM, reed kotler <rkotler at mips.com> wrote:
> >
> //===----------------------------------------------------------------------===//
> > // FalseParser
> >
> //===----------------------------------------------------------------------===//
> > class FalseParser : public parser<bool> {
> >  public:
> >   explicit FalseParser(Option &O) : parser<bool>(O) { }
> >
> >   // parse - Return true on error.
> >   bool parse(cl::Option& O, StringRef ArgName, StringRef Arg, bool& Val)
> {
> >     if (cl::parser<bool>::parse(O, ArgName, Arg, Val))
> >       return false;
> >     Val = false;
> >     return false;
> >   }
> > };
> >
> > I don't know the history of this. I'm just starting to do some mclinker
> work
> > to add the new mips r6 relocations to it.
> >
> >
> > On 03/18/2015 07:00 PM, David Blaikie wrote:
> >
> >
> >
> > On Wed, Mar 18, 2015 at 6:48 PM, reed kotler <rkotler at mips.com> wrote:
> >>
> >> Hi David,
> >>
> >> Is there a reason that we need to have "final" for parser<bool> ???
> >
> >
> > Clang has a (reasonable) warning for types with virtual functions and a
> > non-virtual dtor. This warning is suppressed if the dtor is protected or
> the
> > class is final (since in the first case it's clear that the user intends
> not
> > to destroy objects via base pointers, only derived ones - and in the
> second
> > case there's no risk of derived classes, so public access to the dtor is
> > safe even without virtual dispatch.
> >
> > Since the parser hierarchy never needed polymorphic destruction (all
> > instances are concrete instances of derived classes owned and destroyed
> > directly, not via base pointers) this seemed like a fine way to structure
> > the API.
>
> --
> Simon Atanasyan
>
-------------- next part --------------
An HTML attachment was scrubbed...
URL: <http://lists.llvm.org/pipermail/llvm-dev/attachments/20150319/0651f87c/attachment.html>