[LLVMdev] Final added to parser<bool>

[David Blaikie]

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


>
>
>    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/082172f6/attachment.html>