[cfe-dev] C++ typedef merging
Chris Lattner
clattner at apple.com
Tue Dec 18 21:56:47 PST 2007
>> -----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
More information about the cfe-dev
mailing list