[LLVMdev] Final added to parser<bool>
Reed Kotler
reed.kotler at imgtec.com
Thu Mar 19 10:07:37 PDT 2015
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.
We have a different point of view here.
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/27a80d11/attachment.html>
More information about the llvm-dev
mailing list