[cfe-dev] C++ typedef merging

[Chris Lattner]

>> -----Message d'origine-----
>> De : Chris Lattner [mailto:clattner at apple.com]
>>
>> which seems a bit simpler.  Also, can you come up with an example
>> which would hit the TagDecl case here yet?
>
> Sorry for the wait.
>
> Here is a slightly modified patch following your indications.
>
> We can't it the TagDecl because:
>
> The function is called in Sema::ActOnDeclarator On line 726 of  
> semadecl.cpp
> only if a previous decl was found in the ordinary namespace  
> (ScopedDecl
> *PrevDecl = LookupScopedDecl(II, Decl::IDNS_Ordinary,...). However,  
> the type
> wich would cause to hit the tagdecl live in the Tag namespace  
> IDNS_Tag. So
> until we implement the basic C++ name lookup, I don't think we can  
> hit the
> tagdecl case.

Ok, I'm somewhat uncomfortable with this patch.  It basically relies  
on implementing correct lookup for C++ names, but that hasn't been  
done yet.  If we had that part first, I'd be more happy with this  
one.  Regardless, some more specific thoughts:

@@ -17,6 +17,7 @@
  #include "clang/AST/Decl.h"
  #include "clang/AST/Expr.h"
  #include "clang/AST/Type.h"
+#include "clang/Lex/Preprocessor.h"

Please remove.

+
+  // Redefinitions of typedef are ok in C++ as long as the subsequent  
defi-
+  // nition is the same type as the first definition: C++ 7.1.3  
[dcl.typedef]
+  if(PP.getLangOptions().CPlusPlus) {
+    // FIXME: only valid in non-class scope, detect class-scope

Nice.  But PP.getLangOptions() -> getLangOptions()

+    if(TagDecl* oldTag = dyn_cast<TagDecl>(OldD)) {
+      oldQT = Context.getTagDeclType( oldTag );
+    } else if(TypedefDecl* oldTypeDef = dyn_cast<TypedefDecl>(OldD)) {
+      oldQT = oldTypeDef->getUnderlyingType().getCanonicalType();

Every time I see this, it is unclear to me why you care specifically  
about tagdecl and typedef decls for the previous thing.  I have to go  
around in circles to figure out why.  To resolve this please add a  
comment like:

// If the previous decl was a decl of a type (not a variable,  
namespace, etc) then
// it can potentially be merged with this typedef.  The two things  
that declare
// types are tagdecls and typedefs.  If we have one, get the type it  
is declaring.

+    } else {
+      Diag(OldD->getLocation(), diag::err_previous_definition);
+      return New;
+    }

You don't need this, just remove it and fall through to the error  
checking below it.


+    if(!oldQT.isNull() &&
+      oldQT == New->getUnderlyingType().getCanonicalType()) {
+      return New; // Ok, new typedef don't change the canonical type,  
valid C++
+    }

I'd suggest:

      // If the previous decl was for a type and if it matches, return  
success.
+    if(oldQT == New->getUnderlyingType().getCanonicalType())
+      return New;



Please remove this code:

+    // If we are here, then there is an error: emit the appropriate  
diagnostic
+    if(oldQT.isNull()) {
+      Diag(New->getLocation(),
+           diag::err_redefinition_different_kind,New->getName());
+    } else {
+      Diag(New->getLocation(), diag::err_redefinition, New->getName());
+    }
+    Diag(OldD->getLocation(), diag::err_previous_definition);
+    // we must return somethings, to allow the parsing to continue.
+    // The easiest and probably best error recovery is to simply return
+    // the new typedef.
+    return New;

It is both unnecessary (the fall through case handles this) and  
incorrect for ObjC++.

-Chris