[LLVMdev] Final added to parser<bool>

[David Blaikie]

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.


> and Google uses mclinker
>

So I've been told, though I hadn't even heard of mclinker until this email
thread.


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


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


> 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/2a92aac1/attachment.html>