[LLVMdev] Final added to parser<bool>

[Reed Kotler]

On 03/19/2015 08:55 AM, David Blaikie wrote:
>
>
> On Thu, Mar 19, 2015 at 4:30 AM, Reed Kotler <Reed.Kotler at imgtec.com 
> <mailto: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.
There are lots of valid reasons to want to be able to derive classes; 
that's why C++ lets you do that. :)

I don't think that LLVM judging MCLinker coding is relevant here. Lots 
of projects use LLVM now and changing the API in this way
does not have justification other than to deal with some clang warning 
issues that can be solved in other ways.

I don't see any justification for deciding once and for all that there 
are no valid reasons for deriving from these classes
and to use a c++ feature to enforce that.

As I said earlier, had mclinker been in the tools subdirectory of LLVM 
then the change the CommandLine.h would not have even been allowed.



>
>     ________________________________________
>     From: Simon Atanasyan [simon at atanasyan.com
>     <mailto: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 <mailto: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
>     <mailto: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
>     <mailto: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/94aa19f7/attachment.html>