[LLVMdev] Final added to parser<bool>

[David Blaikie]

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.


>
>
>
>>
>>    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/101ba114/attachment.html>