<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:57 AM, David Blaikie
wrote:<br>
</div>
<blockquote
cite="mid:CAENS6EsSVUM88PBJ-xM6dTond_pkJB5gtncb7e+cQwaG+go=Fg@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:52 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:38 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: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>
<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><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><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>
</span> 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.<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"><span><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>
</span> 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.</div>
</blockquote>
<div><br>
I rather like the notion of not paying for what one
doesn't use (a common cliche/principle of C++) - until
there's a use case (& granted, use cases don't need to
be in-tree, but it'd need to be justified & it's not
clear mclinker's use is) I'd err on the side of not paying
anything for something not used.<br>
</div>
</div>
</div>
</div>
</blockquote>
<br>
IMHO, I think it's a common LLVM cliche to worry about non
measurable performance issues at the cost of making the code more
complex and more difficult to maintain.<br>
<br>
I don't think it's a common cliche/principle of C++ to not pay for
things you don't use except as the language gives you some of that
for free (in tow with a good optimizing compiler).<br>
<br>
I prefer to make things more general if making them more specific
adds complexity and/or makes them less useful later.<br>
<br>
Adding complexity to get increased performance/space savings to me
is what needs to be justified.<br>
Rather than just have virtual destructors here, there are lots of
complex C++ issues at play which in the end could only be solved by
another<br>
complexity of adding the "final" keyword which now restricts the
usefulness of the classes.<br>
<br>
We have a different point of view here. <br>
<br>
Reed<br>
<br>
<br>
<blockquote
cite="mid:CAENS6EsSVUM88PBJ-xM6dTond_pkJB5gtncb7e+cQwaG+go=Fg@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>
<div><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>
</div>
</div>
</div>
</blockquote>
</div>
<br>
</div>
</div>
</blockquote>
<br>
</body>
</html>