[cfe-dev] using declaration questions

[Douglas Gregor]

On Jun 19, 2009, at 12:48 PM, John Thompson wrote:


>
> Generally, I suggest that you post patches to the list, because  
> there are others who might also provide reviews. Some comments follow.
>
> Sorry Doug, I copied both you and the list, which is redundant, so  
> I'll just use the list from now on.

Thanks. All of the Clang developers follow this list closely.

> Index: tools/clang/include/clang/Basic/Builtins.h
>
> ===================================================================
> --- tools/clang/include/clang/Basic/Builtins.h  (revision 73735)
> +++ tools/clang/include/clang/Basic/Builtins.h  (working copy)
> @@ -31,6 +31,7 @@
>
>  enum ID {
>   NotBuiltin  = 0,      // This is not a builtin function.
>  #define BUILTIN(ID, TYPE, ATTRS) BI##ID,
> +#undef alloca
>  #include "clang/Basic/Builtins.def"
>   FirstTSBuiltin
>  };
>
> I thought this build issue was fixed. Do you still need this?
>
> Actually, I didn't see my prior change in the update, but for some  
> reason, with my original change, I still was getting the error, even  
> after a full rebuild.  Perhaps it was a weirdness with Visual  
> Studio.  But moving the undefine up into the header fixed it,  
> again.  Do you think I should redefine it (#define abort _abort) for  
> Windows after the Buildtins.def include?

Ugh. What a pain. If we need this to build on Windows, that's fine,  
but I'd like to to have the appropriate #ifdef's to keep this a  
Windows-only hack.

> Index: tools/clang/lib/AST/DeclBase.cpp
> ===================================================================
> --- tools/clang/lib/AST/DeclBase.cpp    (revision 73735)
> +++ tools/clang/lib/AST/DeclBase.cpp    (working copy)
> @@ -164,6 +164,7 @@
>
>     case ParmVar:
>     case OriginalParmVar:
>     case NonTypeTemplateParm:
> +    case Using:
>     case ObjCMethod:
>     case ObjCContainer:
>     case ObjCCategory:
>
> This puts all using declarations into the ordinary namespace.  
> However, what happens if the using declaration refers to something  
> not in the ordinary namespace, e.g.,
>
>  namespace N {
>    struct X;
>  }
>
>  using N::X;
>  void X(int); // okay
>
>  struct X *x; // okay
>
> I guess my expectation was that UsingDecl would bitwise-OR the  
> identifier namespace values of all of the things that it points to.
>
> Sorry, I blindly put that in not understanding it while searching  
> for places where decl types were used.  I'm afraid I still don't  
> understand it, so I'll have to study it some more as part of working  
> on the semantics, unless you have an easy fix.  I've left what I had  
> in for now, to keep the test file working.
>

The easy fix, which is good enough for now, is to get the target  
declaration's identifier namespace. Eventually, we'll have to cope  
with a using declaration that names both a tag and a value; then it'll  
be more fun :)

>
> +  // Parse namespace-name.
> +  if (SS.isInvalid() || Tok.isNot(tok::identifier)) {
> +    Diag(Tok, diag::err_expected_namespace_or_type_name);
> +    // If there was invalid namespace name, skip to end of decl,  
> and eat ';'.
> +    SkipUntil(tok::semi);
> +    return DeclPtrTy();
> +  }
>
> "expected namespace or type name" seems like the wrong diagnostic. I  
> think we should separate the two cases:
>        - If the scope specifier is invalid, we've already emitted an  
> error so we can just return.
>
>        - If the token following the scope specifier is not an  
> identifier, we can say something like "expected an identifier in  
> using directive". It would also be really great if we detected the  
> case of a template-id (e.g., "using std::vector<int>;") specifically  
> (C++ [namespace.udecl]p5), because it's such an easy error to make.  
> This is a case where we can provide a very specific diagnostic.
> I've taken a stab at fixing this in the new patch.

Looks good.

> +  // Lookup target name.
> +  LookupResult R = LookupParsedName(S, &SS, TargetName,
> +                                    LookupOrdinaryName, false);
> +
> +  if (NamedDecl *NS = R) {
> +    if (IsTypeName && !isa<TypeDecl>(NS)) {
> +      Diag(IdentLoc, diag::err_using_typename_non_type);
> +      return DeclPtrTy();
> +    }
>
> Good, good. Once we've emitted the error, I don't think we have to  
> return, because we can recover from this particular error easily.
> So just delete the return statement?  I've added that to the new  
> patch.

That's fine.

> Thanks, Doug.  I've enclosed a new patch, usingdecl3.patch.  I've  
> also added your examples to the test file, the last part of which is  
> for later work.  I left in a couple of the old changes to SemaDecl  
> to keep the test file working, but you can ignore them if you want.   
> Otherwise I'll take them out when I have the substitution working.

Did you run the Clang test suite with this patch? I'm seeing several  
new failures with your patch:

	/Volumes/Data/dgregor/Projects/llvm/tools/clang/test/Parser/cxx-using- 
directive.cpp
	/Volumes/Data/dgregor/Projects/llvm/tools/clang/test/SemaCXX/member- 
name-lookup.cpp
	/Volumes/Data/dgregor/Projects/llvm/tools/clang/test/SemaCXX/ 
namespace-alias.cpp
	/Volumes/Data/dgregor/Projects/llvm/tools/clang/test/SemaCXX/using- 
directive.cpp

I fixed the first one (it was just a changed error message) and the  
non-semantic issue in the last one. However, I ended up removing the  
new code in SemaLookup, which was causing the other failures. At a  
minimum, all existing tests will have to pass before we can put in the  
name-lookup part of your patch.

> I hope my working on Clang is working out for you guys, and not  
> taking more hand-holding than it is worth.  I fear I might be in a  
> bit over my head, but I'd definitely like to and help out, if I can.

There's always a ramp-up, especially for C++. We know that :)

Some nit-picks follow. I've committed everything except the SemaLookup  
part; thanks! The commit is here:

	http://lists.cs.uiuc.edu/pipermail/cfe-commits/Week-of-Mon-20090615/018333.html

+def err_expected_ident_in_using : Error<"expected an identifier in  
using directive">;
+def err_unexpected_template_inst_in_using : Error<"use of template  
instantiation in using directive not allowed">;

Please keep lines to <= 80 columns.

And, to be *really* pedantic, the template-id refers to a template  
specialization, not a template instantiation. I've fixed the wording.

Index: tools/clang/include/clang/Parse/Action.h
===================================================================
--- tools/clang/include/clang/Parse/Action.h	(revision 73735)
+++ tools/clang/include/clang/Parse/Action.h	(working copy)
@@ -912,7 +912,7 @@
    /// ActOnNamespaceAliasDef - This is called when a namespace alias  
definition
    /// is parsed.
    virtual DeclPtrTy ActOnNamespaceAliasDef(Scope *CurScope,
-                                           SourceLocation NamespaceLoc,
+                                           SourceLocation TargetLoc,
                                             SourceLocation AliasLoc,
                                             IdentifierInfo *Alias,
                                             const CXXScopeSpec &SS,

Why the name change here?

+  // Check nested-name specifier.
+  if (SS.isInvalid()) {
+    return DeclPtrTy();
+  }

Missing a:

   SkipUntil(tok::semi);

before the return.

In the test case:

   // Should have some errors here.  Waiting for implementation.
   void X(int);
   struct X *x;

Our convention is to put "FIXME:" in the comment for anything that  
needs to be fixed later.

   - Doug