<div dir="ltr"><br><div class="gmail_extra"><br><div class="gmail_quote">On Thu, Mar 19, 2015 at 4:30 AM, Reed Kotler <span dir="ltr"><<a href="mailto:Reed.Kotler@imgtec.com" target="_blank">Reed.Kotler@imgtec.com</a>></span> wrote:<br><blockquote class="gmail_quote" style="margin:0 0 0 .8ex;border-left:1px #ccc solid;padding-left:1ex">One could argue that mclinker is doing something good or not by how it's using this class<br>
but I don't see the need for parser<bool> to be final. That is a subjective opinion that mclinker needs to<br>
be changed.<br>
<br>
I think that "final" was added to some of these command line classes to avoid some kind of clang warning.<br>
<br>
That seems wrong to me that the tools are dictating something in an API.<br></blockquote><div><br>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.<br><br>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).<br><br>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.<br> </div><blockquote class="gmail_quote" style="margin:0 0 0 .8ex;border-left:1px #ccc solid;padding-left:1ex">There may be other valid reasons to create derived classes.<br>
<br>
I think we should remove the "final" attribute on those classes and fix the clang warning issue in some<br>
better way than changing the API.<br>
<br>
If mclinker had been part of the llvm project tools tree that would not have been allowed even.<br></blockquote><div><br>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.<br> </div><blockquote class="gmail_quote" style="margin:0 0 0 .8ex;border-left:1px #ccc solid;padding-left:1ex">
<br>
________________________________________<br>
From: Simon Atanasyan [<a href="mailto:simon@atanasyan.com">simon@atanasyan.com</a>]<br>
Sent: Wednesday, March 18, 2015 9:35 PM<br>
To: Reed Kotler<br>
Cc: David Blaikie; Simon Atanasyan; Rich Fuhler; <a href="mailto:llvmdev@cs.uiuc.edu">llvmdev@cs.uiuc.edu</a><br>
Subject: Re: [LLVMdev] Final added to parser<bool><br>
<div class="HOEnZb"><div class="h5"><br>
Hi Reed,<br>
<br>
The FalseParser has a rather strange design. It's purpose to parse<br>
options like -no-to-do-something and always return false even if a<br>
user provides -no-to-do-something=no. That should be fixed on the<br>
MCLinker side.<br>
<br>
On Thu, Mar 19, 2015 at 5:14 AM, reed kotler <<a href="mailto:rkotler@mips.com">rkotler@mips.com</a>> wrote:<br>
> //===----------------------------------------------------------------------===//<br>
> // FalseParser<br>
> //===----------------------------------------------------------------------===//<br>
> class FalseParser : public parser<bool> {<br>
> public:<br>
> explicit FalseParser(Option &O) : parser<bool>(O) { }<br>
><br>
> // parse - Return true on error.<br>
> bool parse(cl::Option& O, StringRef ArgName, StringRef Arg, bool& Val) {<br>
> if (cl::parser<bool>::parse(O, ArgName, Arg, Val))<br>
> return false;<br>
> Val = false;<br>
> return false;<br>
> }<br>
> };<br>
><br>
> I don't know the history of this. I'm just starting to do some mclinker work<br>
> to add the new mips r6 relocations to it.<br>
><br>
><br>
> On 03/18/2015 07:00 PM, David Blaikie wrote:<br>
><br>
><br>
><br>
> On Wed, Mar 18, 2015 at 6:48 PM, reed kotler <<a href="mailto:rkotler@mips.com">rkotler@mips.com</a>> wrote:<br>
>><br>
>> Hi David,<br>
>><br>
>> Is there a reason that we need to have "final" for parser<bool> ???<br>
><br>
><br>
> Clang has a (reasonable) warning for types with virtual functions and a<br>
> non-virtual dtor. This warning is suppressed if the dtor is protected or the<br>
> class is final (since in the first case it's clear that the user intends not<br>
> to destroy objects via base pointers, only derived ones - and in the second<br>
> case there's no risk of derived classes, so public access to the dtor is<br>
> safe even without virtual dispatch.<br>
><br>
> Since the parser hierarchy never needed polymorphic destruction (all<br>
> instances are concrete instances of derived classes owned and destroyed<br>
> directly, not via base pointers) this seemed like a fine way to structure<br>
> the API.<br>
<br>
--<br>
Simon Atanasyan<br>
</div></div></blockquote></div><br></div></div>