[LLVMdev] Final added to parser<bool>
Reed Kotler
reed.kotler at imgtec.com
Thu Mar 19 10:26:06 PDT 2015
On 03/19/2015 10:12 AM, David Blaikie wrote:
>
>
> On Thu, Mar 19, 2015 at 10:07 AM, Reed Kotler <reed.kotler at imgtec.com
> <mailto: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 <mailto: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 <mailto: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
>>>> <mailto: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.
Of course. Otherwise there could not be horse races. :)
>
> - 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
>>>>> <mailto: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
>>>>>> <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.
>>>>>
>>>>>
>>>>> 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
>>>>>> <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/e39e4db6/attachment.html>
More information about the llvm-dev
mailing list