[LLVMdev] Final added to parser<bool>

[David Blaikie]

On Thu, Mar 19, 2015 at 9:05 AM, Reed Kotler <reed.kotler at imgtec.com> wrote:

>  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>
> 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.
>

It seems like it is to me - when refactoring code we look at the use cases
and judge what the right way to handle those use cases is. One of those
ways is to fix/change/remove the use-cases that seem
problematic/unnecessary/incorrect.


> 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 think the practices that the clang warning enforces/encourages are good
ones, otherwise I would've fixed/changed/disabled the warning. We've used
these practices/this warning for a while now & it seems to suit LLVM's use
cases pretty well so far.


> 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]
>> 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/f3f8ac65/attachment.html>