<div dir="ltr"><br><div class="gmail_extra"><br><div class="gmail_quote">On Thu, Mar 19, 2015 at 9:18 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">
<div>Well, you are an mclinker contributor
</div></div></blockquote><div><br>Me personally? Not that I know of.<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"><div> and Google uses mclinker</div></div></blockquote><div><br>So I've been told, though I hadn't even heard of mclinker until this email thread.<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"><div> and now it's broken as the result of your
change.<br></div></div></blockquote><div><br>This is a pretty standard state of affairs for LLVM-using projects, LLVM's APIs change continuously. <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"><div>I still don't see any justification to making a change in a public
interface that is used by other non LLVM projects<br>
to fix some issue with clang warnings. People should be able to
derive from those classes. I can't understand<br>
your reasoning as to why these classes must be final.<br></div></div></blockquote><div><br>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).<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"><div>I was kind of surprised that there are no mclinker build bots that
would have picked this up right away.<div><div class="h5"><br>
<br>
On 03/19/2015 09:08 AM, David Blaikie wrote:<br>
</div></div></div><div><div class="h5">
<blockquote type="cite">
<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>
<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><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>
</blockquote>
<br>
</div></div></div>
</blockquote></div><br></div></div>