Hi David,
Is there a reason that we need to have "final" for parser<bool> ???
This breaks the compilation of mclinker which derives a class from this.
In file included from /home/rkotler/workspace/mclinker/lib/Support/CommandLine.cpp:9:0:
/home/rkotler/workspace/mclinker/include/mcld/Support/CommandLine.h:49:7: error: cannot derive from ‘final’ base ‘llvm:
:parser<bool>’ in derived type ‘llvm::FalseParser’
make[2]: *** [Support/CommandLine.o] Error 1
make[2]: Leaving directory `/home/rkotler/mclinker_build/lib'
make[1]: *** [all] Error 2
make[1]: Leaving directory `/home/rkotler/mclinker_build/lib'
make: *** [all-recursive] Error 1
Reed
Hi David,
Is there a reason that we need to have "final" for parser<bool> ???
Clang has a (reasonable) warning for types with virtual functions and a
non-virtual dtor. This warning is suppressed if the dtor is protected or
the class is final (since in the first case it's clear that the user
intends not to destroy objects via base pointers, only derived ones - and
in the second case there's no risk of derived classes, so public access to
the dtor is safe even without virtual dispatch.
Since the parser hierarchy never needed polymorphic destruction (all
instances are concrete instances of derived classes owned and destroyed
directly, not via base pointers) this seemed like a fine way to structure
the API.
This breaks the compilation of mclinker which derives a class from this.
In file included from /home/rkotler/workspace/mclinker/lib/Support/
CommandLine.cpp:9:0:
/home/rkotler/workspace/mclinker/include/mcld/Support/CommandLine.h:49:7:
error: cannot derive from ‘final’ base ‘llvm::parser<bool>’ in derived
type ‘llvm::FalseParser’
Why is it being derived from? If it's just a typedef that's required, it
might be more appropriate to use a typedef instead of derivation.
//===----------------------------------------------------------------------===//
// FalseParser
//===----------------------------------------------------------------------===//
class FalseParser : public parser<bool> {
public:
explicit FalseParser(Option &O) : parser<bool>(O) { }
// parse - Return true on error.
bool parse(cl::Option& O, StringRef ArgName, StringRef Arg, bool& Val) {
if (cl::parser<bool>::parse(O, ArgName, Arg, Val))
return false;
Val = false;
return false;
}
};
I don't know the history of this. I'm just starting to do some mclinker work to add the new mips r6 relocations to it.
Hi Reed,
The FalseParser has a rather strange design. It's purpose to parse
options like -no-to-do-something and always return false even if a
user provides -no-to-do-something=no. That should be fixed on the
MCLinker side.
One could argue that mclinker is doing something good or not by how it's using this class
but I don't see the need for parser<bool> to be final. That is a subjective opinion that mclinker needs to
be changed.
I think that "final" was added to some of these command line classes to avoid some kind of clang warning.
That seems wrong to me that the tools are dictating something in an API.
There may be other valid reasons to create derived classes.
I think we should remove the "final" attribute on those classes and fix the clang warning issue in some
better way than changing the API.
If mclinker had been part of the llvm project tools tree that would not have been allowed even.
One could argue that mclinker is doing something good or not by how it's
using this class
but I don't see the need for parser<bool> to be final. That is a
subjective opinion that mclinker needs to
be changed.
I think that "final" was added to some of these command line classes to
avoid some kind of clang warning.
That seems wrong to me that the tools are dictating something in an API.
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.
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).
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.
There may be other valid reasons to create derived classes.
I think we should remove the "final" attribute on those classes and fix
the clang warning issue in some
better way than changing the API.
If mclinker had been part of the llvm project tools tree that would not
have been allowed even.
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.
There are lots of valid reasons to want to be able to derive classes; that’s why C++ lets you do that. 
I don’t think that LLVM judging MCLinker coding is relevant here. Lots of projects use LLVM now and changing the API in this way does not have justification other than to deal with some clang warning issues that can be solved in other ways. I don’t see any justification for deciding once and for all that there are no valid reasons for deriving from these classes and to use a c++ feature to enforce that. 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.
One could argue that mclinker is doing something good or not by how it's
using this class
but I don't see the need for parser<bool> to be final. That is a
subjective opinion that mclinker needs to
be changed.
I think that "final" was added to some of these command line classes to
avoid some kind of clang warning.
That seems wrong to me that the tools are dictating something in an API.
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.
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).
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.
There may be other valid reasons to create derived classes.
I think we should remove the "final" attribute on those classes and fix
the clang warning issue in some
better way than changing the API.
If mclinker had been part of the llvm project tools tree that would not
have been allowed even.
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.
There are lots of valid reasons to want to be able to derive classes;
that's why C++ lets you do that.
I don't think that LLVM judging MCLinker coding is relevant here.
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.
Lots of projects use LLVM now and changing the API in this way
does not have justification other than to deal with some clang warning
issues that can be solved in other ways.
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.
Well, you are an mclinker contributor and Google uses mclinker and now it's broken as the result of your change.
I still don't see any justification to making a change in a public interface that is used by other non LLVM projects
to fix some issue with clang warnings. People should be able to derive from those classes. I can't understand
your reasoning as to why these classes must be final.
I was kind of surprised that there are no mclinker build bots that would have picked this up right away.
Well, you are an mclinker contributor
Me personally? Not that I know of.
and Google uses mclinker
So I've been told, though I hadn't even heard of mclinker until this email
thread.
and now it's broken as the result of your change.
This is a pretty standard state of affairs for LLVM-using projects, LLVM's
APIs change continuously.
I still don't see any justification to making a change in a public
interface that is used by other non LLVM projects
to fix some issue with clang warnings. People should be able to derive
from those classes. I can't understand
your reasoning as to why these classes must be final.
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).
I still don't see any justification to making a change in a public interface
that is used by other non LLVM projects
to fix some issue with clang warnings. People should be able to derive from
those classes. I can't understand
your reasoning as to why these classes must be final.
From my point of view this class is a good candidate to be final. I
cannot realize a real test case when somebody needs to extend parsing
of boolean command line flags. If MCLinker had been part of the LLVM
project the FalseParser design would not have passed review.
I was kind of surprised that there are no mclinker build bots that would
have picked this up right away.
MCLinker does not use LLVM ToT. It migrates to the fresh version of
LLVM from time to time.
Simon Atanasyan
Sorry. I thought i had seen your name in an mclinker commit. It’s another linker (we don’t have enough of them ) . It’s used to run on mobile devices and has been designed with that criteria in mind. (At least that is my understanding). Probably then this is a configuration issue I need to take up with mclinker folks. Their build instructions have you just download tip of tree from svn and that clearly is not guaranteed to work. Probably their wiki should list the version from LLVM to download. Isn’t that a problem with the design of this class? (or these classes)?
I still don't see any justification to making a change in a public interface
that is used by other non LLVM projects
to fix some issue with clang warnings. People should be able to derive from
those classes. I can't understand
your reasoning as to why these classes must be final.
From my point of view this class is a good candidate to be final. I
cannot realize a real test case when somebody needs to extend parsing
of boolean command line flags. If MCLinker had been part of the LLVM
project the FalseParser design would not have passed review.
Isn't this a problem without how these classes were designed as opposed to
a problem with someone wanting to derive from ?
I was kind of surprised that there are no mclinker build bots that would
have picked this up right away.
MCLinker does not use LLVM ToT. It migrates to the fresh version of
LLVM from time to time.
Seems like we need to fix the mclinker wiki so that people know which version to use.
The mclinker instructions just have one checking out from tip of tree.
Well, you are an mclinker contributor
Me personally? Not that I know of.
Sorry. I thought i had seen your name in an mclinker commit.
and Google uses mclinker
So I've been told, though I hadn't even heard of mclinker until this email
thread.
It's another linker (we don't have enough of them ) . It's used to run
on mobile devices
and has been designed with that criteria in mind. (At least that is my
understanding).
and now it's broken as the result of your change.
This is a pretty standard state of affairs for LLVM-using projects, LLVM's
APIs change continuously.
Probably then this is a configuration issue I need to take up with
mclinker folks.
Their build instructions have you just download tip of tree from svn and
that clearly is not guaranteed to work.
Probably their wiki should list the version from LLVM to download.
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)
I still don't see any justification to making a change in a public
interface that is used by other non LLVM projects
to fix some issue with clang warnings. People should be able to derive
from those classes. I can't understand
your reasoning as to why these classes must be final.
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).
Isn't that a problem with the design of this class? (or these classes)?
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.
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. Is it really necessary to worry about the overhead of virtual destructors for command line processing classes? It makes the classes less generally useful for no measurable benefit.
Well, you are an mclinker contributor
Me personally? Not that I know of.
Sorry. I thought i had seen your name in an mclinker commit.
and Google uses mclinker
So I've been told, though I hadn't even heard of mclinker until this
email thread.
It's another linker (we don't have enough of them ) . It's used to
run on mobile devices
and has been designed with that criteria in mind. (At least that is my
understanding).
and now it's broken as the result of your change.
This is a pretty standard state of affairs for LLVM-using projects,
LLVM's APIs change continuously.
Probably then this is a configuration issue I need to take up with
mclinker folks.
Their build instructions have you just download tip of tree from svn and
that clearly is not guaranteed to work.
Probably their wiki should list the version from LLVM to download.
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)
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.
I still don't see any justification to making a change in a public
interface that is used by other non LLVM projects
to fix some issue with clang warnings. People should be able to derive
from those classes. I can't understand
your reasoning as to why these classes must be final.
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).
Isn't that a problem with the design of this class? (or these classes)?
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.
Is it really necessary to worry about the overhead of virtual destructors
for command line processing classes?
It makes the classes less generally useful for no measurable benefit.
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.
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. 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). I prefer to make things more general if making them more specific adds complexity and/or makes them less useful later. Adding complexity to get increased performance/space savings to me is what needs to be justified. 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 complexity of adding the “final” keyword which now restricts the usefulness of the classes. We have a different point of view here. Reed
Well, you are an mclinker contributor
Me personally? Not that I know of.
Sorry. I thought i had seen your name in an mclinker commit.
and Google uses mclinker
So I've been told, though I hadn't even heard of mclinker until this
email thread.
It's another linker (we don't have enough of them ) . It's used to
run on mobile devices
and has been designed with that criteria in mind. (At least that is my
understanding).
and now it's broken as the result of your change.
This is a pretty standard state of affairs for LLVM-using projects,
LLVM's APIs change continuously.
Probably then this is a configuration issue I need to take up with
mclinker folks.
Their build instructions have you just download tip of tree from svn and
that clearly is not guaranteed to work.
Probably their wiki should list the version from LLVM to download.
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)
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.
I still don't see any justification to making a change in a public
interface that is used by other non LLVM projects
to fix some issue with clang warnings. People should be able to derive
from those classes. I can't understand
your reasoning as to why these classes must be final.
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).
Isn't that a problem with the design of this class? (or these classes)?
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.
Is it really necessary to worry about the overhead of virtual
destructors for command line processing classes?
It makes the classes less generally useful for no measurable benefit.
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.
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.
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).
I prefer to make things more general if making them more specific adds
complexity and/or makes them less useful later.
Adding complexity to get increased performance/space savings to me is what
needs to be justified.
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
complexity of adding the "final" keyword which now restricts the
usefulness of the classes.
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.
We have a different point of view here.
So we do & that's OK.
- David
Of course. Otherwise there could not be horse races.
There are lots of valid reasons to want to be able to derive classes; that's
why C++ lets you do that.
Note that LLVM is a toolchain toolkit. It is not trying to compete
with boost or the standard library.
To do its job LLVM has lots of auxiliary code, like DenseMap or the
option parsing, but there is absolutely no guarantees about their API,
ABI or even existence from one revision to the next.
Cheers,
Rafael