<div dir="ltr"><br><div class="gmail_extra"><br><div class="gmail_quote">On Thu, Mar 19, 2015 at 9:05 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">
<div bgcolor="#FFFFFF" text="#000000"><span class="">
<div>On 03/19/2015 08:55 AM, David Blaikie
wrote:<br>
</div>
<blockquote type="cite">
<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>
</div>
</div>
</div>
</blockquote></span>
There are lots of valid reasons to want to be able to derive
classes; that's why C++ lets you do that. :)<br>
<br>
I don't think that LLVM judging MCLinker coding is relevant here.
</div></blockquote><div><br>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.<br> </div><blockquote class="gmail_quote" style="margin:0 0 0 .8ex;border-left:1px #ccc solid;padding-left:1ex"><div bgcolor="#FFFFFF" text="#000000"> Lots of projects use LLVM now and changing the API in this way<br>
does not have justification other than to deal with some clang
warning issues that can be solved in other ways.<br></div></blockquote><div><br>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.<br> </div><blockquote class="gmail_quote" style="margin:0 0 0 .8ex;border-left:1px #ccc solid;padding-left:1ex"><div bgcolor="#FFFFFF" text="#000000">I don't see any justification for deciding once and for all that
there are no valid reasons for deriving from these classes<br>
and to use a c++ feature to enforce that.<br>
<br>
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.<div><div class="h5"><br>
<br>
<br>
<br>
<blockquote type="cite">
<div dir="ltr">
<div class="gmail_extra">
<div class="gmail_quote">
<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" target="_blank">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" target="_blank">llvmdev@cs.uiuc.edu</a><br>
Subject: Re: [LLVMdev] Final added to parser<bool><br>
<div>
<div><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" target="_blank">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" target="_blank">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>
</blockquote>
<br>
</div></div></div>
</blockquote></div><br></div></div>