<html>
<head>
<meta content="text/html; charset=utf-8" http-equiv="Content-Type">
</head>
<body bgcolor="#FFFFFF" text="#000000">
<div class="moz-cite-prefix">On 03/19/2015 09:38 AM, David Blaikie
wrote:<br>
</div>
<blockquote
cite="mid:CAENS6EtkMCsPhyy=C-BffDrEmgo77ZezcuXJt-wLwO-j=KkzRw@mail.gmail.com"
type="cite">
<meta http-equiv="Content-Type" content="text/html; charset=utf-8">
<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 moz-do-not-send="true"
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
moz-do-not-send="true"
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>
</div>
</div>
</div>
</blockquote>
I have to confess ignorance here because I'm just following their
wiki instructions which are written as if it's TOT compatible with
LLVM.<br>
<blockquote
cite="mid:CAENS6EtkMCsPhyy=C-BffDrEmgo77ZezcuXJt-wLwO-j=KkzRw@mail.gmail.com"
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"><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>
</div>
</div>
</div>
</blockquote>
Is it really necessary to worry about the overhead of virtual
destructors for command line processing classes?<br>
It makes the classes less generally useful for no measurable
benefit.<br>
<br>
<blockquote
cite="mid:CAENS6EtkMCsPhyy=C-BffDrEmgo77ZezcuXJt-wLwO-j=KkzRw@mail.gmail.com"
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>
<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
moz-do-not-send="true"
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
moz-do-not-send="true" 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
moz-do-not-send="true" 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
moz-do-not-send="true" 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
moz-do-not-send="true" 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
moz-do-not-send="true"
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>
</blockquote>
<br>
</body>
</html>