[cfe-commits] r76297 - in /cfe/trunk: include/clang/AST/Decl.h include/clang/AST/Redeclarable.h lib/AST/Decl.cpp
kyrtzidis at apple.com
Sat Jul 18 15:28:58 PDT 2009
On Jul 18, 2009, at 2:45 PM, Chris Lattner wrote:
> On Jul 18, 2009, at 2:05 PM, Argyrios Kyrtzidis wrote:
>>> I don't know what Chris's objection is, but I believe that various
>>> things we do in the Clang codebase would be very fragile in the
>>> face of
>>> multiple inheritance. Most importantly, in passing things back and
>>> between Sema and Parse, we sometimes cast derived classes to void*
>>> then cast the void* to a base pointer. This works as long as the
>>> base is
>>> in the "primary" inheritance chain, i.e. every class in the chain
>>> is the
>>> first in the list of bases. If that isn't the case, the base and
>>> pointers do not have the same value, and the void cast will mess
>>> it up.
>>> Your patch isn't likely to invoke this particular problem, but it
>>> is a
>>> very subtle one, and I believe that it is too much to ask of the
>>> developers to always remember this caveat. It's easier to avoid MI
>>> the first place.
>> How about I make the inheritance private ? The interface will have
>> to be duplicated to FunctionDecl and VarDecl
>> (and any other class that may use Redeclarable) but at least it
>> will only delegate to the implementation of Redeclarable.
> As Sebastian points out, our use isa/dyncast etc and other tricks
> for efficient memory use sometimes run afoul of multiple
> inheritance. I generally try to avoid it where possible in any
> case, except where there is no other good option (e.g. around
> DeclContext, which I still don't think is a good idea ;-). Private
> inheritance doesn't help either.
Well, as I previously said, Redeclarable is an implementation detail,
you won't get a pointer to it for using or casting, so isa/dyn_cast
doesn't apply. Is there any other trick that is going to be affected
by it ?
> Is there any reason not to introduce a new class that these things
> directly inherit from?
The other "candidate" users are FunctionTemplateDecl and
ClassTemplateDecl, and the common ancestor with FunctionDecl and
VarDecl is NamedDecl, so we would have something like this:
Which is obvious wrong, not all of these (and their subclasses) are
And in general, doing a dyn_cast<RedeclarableDecl> or
isa<Redeclarable> is not very useful. It is only interesting for
libIndex but doing a Decl::redecls_begin()/redecls_end() covers that
more than enough without making an intrunsive change like this.
More information about the cfe-commits