<div dir="ltr"><br><div class="gmail_extra"><br><div class="gmail_quote">On Thu, Mar 19, 2015 at 9:34 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 09:24 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 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>
</div>
</div>
</div>
</blockquote></span>
Sorry. I thought i had seen your name in an mclinker commit.<span class=""><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">
<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>
</div>
</div>
</div>
</blockquote></span>
It's another linker (we don't have enough of them :) ) . It's used
to run on mobile devices<br>
and has been designed with that criteria in mind. (At least that is
my understanding).<span class=""><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">
<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>
</div>
</div>
</div>
</blockquote>
<blockquote type="cite">
<div dir="ltr">
<div class="gmail_extra">
<div class="gmail_quote">
<div> </div>
</div>
</div>
</div>
</blockquote></span>
Probably then this is a configuration issue I need to take up with
mclinker folks.<br>
Their build instructions have you just download tip of tree from svn
and that clearly is not guaranteed to work.<br>
Probably their wiki should list the version from LLVM to download.</div></blockquote><div><br>I don't know what the deal is with mclinker's strategy here, but some out of tree projects follow llvm - the nature of that is that it'll break from time to time (just as in-tree projects break with LLVM API changes (not many people checkout/build some subprojects like polly, etc)) - for projects that follow ToT, not any particular snapshot revision, it's just a matter of who syncs up first & sees the break - everyone usually takes some responsibility to fix things up if they break. (and/or loop the right people in if you sync up & it's broken and you don't know how to fix it & have a chat about how to fix it)<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"><span class=""><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">
<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>
</div>
</div>
</div>
</blockquote></span>
Isn't that a problem with the design of this class? (or these
classes)?</div></blockquote><div><br>Not really - not for LLVM's use cases (which are the ones the API is designed for, of course). We could make the dtor virtual instead - but that's unnecessary overhead compared to just ensuring (by use of protected dtors and final classes) that the class can't be destroyed polymorphically anyway. It's a legitimate design to just disallow it rather than adding overhead to allow it when it's not needed by the use cases we have.<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><div class="h5"><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">
<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><br>
<br>
On 03/19/2015 09:08 AM, David Blaikie wrote:<br>
</div>
</div>
</div>
<div>
<div>
<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>
</blockquote>
<br>
</div></div></div>
</blockquote></div><br></div></div>