[cfe-commits] r76297 - in /cfe/trunk: include/clang/AST/Decl.h include/clang/AST/Redeclarable.h lib/AST/Decl.cpp

[Argyrios Kyrtzidis]

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  
>>> forth
>>> between Sema and Parse, we sometimes cast derived classes to void*  
>>> and
>>> 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  
>>> derived
>>> 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  
>>> Clang
>>> developers to always remember this caveat. It's easier to avoid MI  
>>> in
>>> 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:

NamedDecl -
              RedeclarableDecl -
				ValueDecl-
						EnumConstantDecl
						FunctionDecl -
						FieldDecl-
						VarDecl-
				TemplateDecl-
						FunctionTemplateDecl
						ClassTemplateDecl
						TemplateTemplateParmDecl

Which is obvious wrong, not all of these (and their subclasses) are  
"redeclarable".

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  
need
more than enough without making an intrunsive change like this.

-Argiris