<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 10:12 AM, David Blaikie
wrote:<br>
</div>
<blockquote
cite="mid:CAENS6EsY9NY2w3fgGGgvzZJ75cc1OWjv5D8=BM0HO7=Rzck3qA@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 10:07 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>
<div class="h5">
<div>On 03/19/2015 09:57 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: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>
<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><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>
</div>
</div>
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>
</div>
</blockquote>
<div><br>
This doesn't seem to be particularly complicated to me
& is an idiom seen in multiple places in the LLVM
codebase, and reinforced by a clang warning, so it's
certainly one I'd expect LLVM developers to have a passing
familiarity with, or the ability to get familiar with it
fairly easily.<br>
</div>
</div>
</div>
</div>
</blockquote>
<blockquote
cite="mid:CAENS6EsY9NY2w3fgGGgvzZJ75cc1OWjv5D8=BM0HO7=Rzck3qA@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">We have a different
point of view here. <br>
</div>
</blockquote>
<div><br>
So we do & that's OK.<br>
</div>
</div>
</div>
</div>
</blockquote>
Of course. Otherwise there could not be horse races. :)<br>
<br>
<blockquote
cite="mid:CAENS6EsY9NY2w3fgGGgvzZJ75cc1OWjv5D8=BM0HO7=Rzck3qA@mail.gmail.com"
type="cite">
<div dir="ltr">
<div class="gmail_extra">
<div class="gmail_quote">
<div><br>
- David<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="HOEnZb"><font
color="#888888"> <br>
Reed</font></span>
<div>
<div class="h5"><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">
<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>
<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>
</div>
</div>
</div>
</blockquote>
</div>
<br>
</div>
</div>
</blockquote>
<br>
</body>
</html>