[LLVMdev] Final added to parser<bool>

David Blaikie dblaikie at gmail.com
Thu Mar 19 10:12:47 PDT 2015 

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

>  On 03/19/2015 09:57 AM, David Blaikie wrote:
>
>
>
> On Thu, Mar 19, 2015 at 9:52 AM, Reed Kotler <reed.kotler at imgtec.com>
> wrote:
>
>>  On 03/19/2015 09:38 AM, David Blaikie wrote:
>>
>>
>>
>> On Thu, Mar 19, 2015 at 9:34 AM, Reed Kotler <reed.kotler at imgtec.com>
>> wrote:
>>
>>>  On 03/19/2015 09:24 AM, David Blaikie wrote:
>>>
>>>
>>>
>>> On Thu, Mar 19, 2015 at 9:18 AM, Reed Kotler <reed.kotler at imgtec.com>
>>> wrote:
>>>
>>>>  Well, you are an mclinker contributor
>>>>
>>>
>>> Me personally? Not that I know of.
>>>
>>>
>>>  Sorry. I thought i had seen your name in an mclinker commit.
>>>
>>>    and Google uses mclinker
>>>>
>>>
>>> So I've been told, though I hadn't even heard of mclinker until this
>>> email thread.
>>>
>>>
>>>  It's another linker (we don't have enough of them :) ) . It's used to
>>> run on mobile devices
>>> and has been designed with that criteria in mind. (At least that is my
>>> understanding).
>>>
>>>     and now it's broken as the result of your change.
>>>>
>>>
>>> This is a pretty standard state of affairs for LLVM-using projects,
>>> LLVM's APIs change continuously.
>>>
>>>
>>>
>>>  Probably then this is a configuration issue I need to take up with
>>> mclinker folks.
>>> Their build instructions have you just download tip of tree from svn and
>>> that clearly is not guaranteed to work.
>>> Probably their wiki should list the version from LLVM to download.
>>>
>>
>> I don't know what the deal is with mclinker's strategy here, but some out
>> of tree projects follow llvm - the nature of that is that it'll break from
>> time to time (just as in-tree projects break with LLVM API changes (not
>> many people checkout/build some subprojects like polly, etc)) - for
>> projects that follow ToT, not any particular snapshot revision, it's just a
>> matter of who syncs up first & sees the break - everyone usually takes some
>> responsibility to fix things up if they break. (and/or loop the right
>> people in if you sync up & it's broken and you don't know how to fix it &
>> have a chat about how to fix it)
>>
>>
>>  I have to confess ignorance here because I'm just following their wiki
>> instructions which are written as if it's TOT compatible with LLVM.
>>
>>
>>>
>>>    I still don't see any justification to making a change in a public
>>>> interface that is used by other non LLVM projects
>>>> to fix some issue with clang warnings. People should be able to derive
>>>> from those classes. I can't understand
>>>> your reasoning as to why these classes must be final.
>>>>
>>>
>>> Because if they're not, it's easy to write bugs with them - especially
>>> in non-tree projects, strangely enough. (if you derive from this type, then
>>> happen to own those derived objects polymorphically, you'll invoke
>>> undefined behavior - by making the type 'final' it removes the ability to
>>> write such bugs).
>>>
>>>
>>>  Isn't that a problem with the design of this class? (or these classes)?
>>>
>>
>> Not really - not for LLVM's use cases (which are the ones the API is
>> designed for, of course). We could make the dtor virtual instead - but
>> that's unnecessary overhead compared to just ensuring (by use of protected
>> dtors and final classes) that the class can't be destroyed polymorphically
>> anyway. It's a legitimate design to just disallow it rather than adding
>> overhead to allow it when it's not needed by the use cases we have.
>>
>>
>>  Is it really necessary to worry about the overhead of virtual
>> destructors for command line processing classes?
>> It makes the classes less generally useful for no measurable benefit.
>>
>
> I rather like the notion of not paying for what one doesn't use (a common
> cliche/principle of C++) - until there's a use case (& granted, use cases
> don't need to be in-tree, but it'd need to be justified & it's not clear
> mclinker's use is) I'd err on the side of not paying anything for something
> not used.
>
>
>
> IMHO, I think it's a common LLVM cliche to worry about non measurable
> performance issues at the cost of making the code more complex and more
> difficult to maintain.
>
> I don't think it's a common cliche/principle of C++ to not pay for things
> you don't use except as the language gives you some of that for free (in
> tow with a good optimizing compiler).
>
> I prefer to make things more general if making them more specific adds
> complexity and/or makes them less useful later.
>
> Adding complexity to get increased performance/space savings to me is what
> needs to be justified.
> Rather than just have virtual destructors here, there are lots of complex
> C++ issues at play which in the end could only be solved by another
> complexity of adding the "final" keyword which now restricts the
> usefulness of the classes.
>

This doesn't seem to be particularly complicated to me & is an idiom seen
in multiple places in the LLVM codebase, and reinforced by a clang warning,
so it's certainly one I'd expect LLVM developers to have a passing
familiarity with, or the ability to get familiar with it fairly easily.


> We have a different point of view here.
>

So we do & that's OK.

- David


>
> Reed
>
>
>
>
>>
>>
>>>
>>>    I was kind of surprised that there are no mclinker build bots that
>>>> would have picked this up right away.
>>>>
>>>>
>>>> On 03/19/2015 09:08 AM, David Blaikie wrote:
>>>>
>>>>
>>>>
>>>> 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/81053ac6/attachment.html>

More information about the llvm-dev mailing list