[LLVMdev] Final added to parser<bool>
Reed Kotler
reed.kotler at imgtec.com
Thu Mar 19 09:18:40 PDT 2015
Well, you are an mclinker contributor and Google uses mclinker and now
it's broken as the result of your change.
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.
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/e04cc198/attachment.html>
More information about the llvm-dev
mailing list