[cfe-dev] using declaration questions

[John Thompson]

>
>
> 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.


>
> 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?


>
>
> 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.


>
> +  // 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.

>
> +  // 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.

>
> Index: tools/clang/lib/Sema/SemaLookup.cpp
> ===================================================================
> --- tools/clang/lib/Sema/SemaLookup.cpp (revision 73735)
> +++ tools/clang/lib/Sema/SemaLookup.cpp (working copy)
> @@ -1151,8 +1151,13 @@
>
>                                Name, NameKind, RedeclarationOnly);
>   }
>
> -  return LookupName(S, Name, NameKind, RedeclarationOnly,
> -                    AllowBuiltinCreation, Loc);
> +  LookupResult result(LookupName(S, Name, NameKind, RedeclarationOnly,
> +                    AllowBuiltinCreation, Loc));
> +  UsingDecl* usingDecl;
> +  if (!RedeclarationOnly && (bool)result &&
> +        ((usingDecl = dyn_cast<UsingDecl>(result.getAsDecl())) != NULL))
> +        return(LookupResult::CreateLookupResult(Context,
> usingDecl->getTargetDecl()));
> +  return(result);
>  }
>
> This is only one of many places where we would have to perform the
> UsingDecl -> target declaration mapping. I think it would be much easier to
> move this actual mapping into CreateLookupResult (or some helper that we use
> instead of CreateLookupResult). That way, there's only one point where we
> have to cope with using declarations.
>
Okay.  I'll work on this after this patch.

>
> We're definitely getting closer. My comments about the parsing side of
> things were pretty trivial; if you want to make those little changes first,
> I can commit that part of your patch to the tree so you can focus on
> semantics (which are significantly tougher).


>        - Doug


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.

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.

-- 
John Thompson
John.Thompson.JTSoftware at gmail.com
-------------- next part --------------
An HTML attachment was scrubbed...
URL: <http://lists.llvm.org/pipermail/cfe-dev/attachments/20090619/fe9abee4/attachment.html>
-------------- next part --------------
A non-text attachment was scrubbed...
Name: usingdecl3.patch
Type: application/octet-stream
Size: 18295 bytes
Desc: not available
URL: <http://lists.llvm.org/pipermail/cfe-dev/attachments/20090619/fe9abee4/attachment.obj>
-------------- next part --------------
A non-text attachment was scrubbed...
Name: cxx-using-declaration.cpp
Type: application/octet-stream
Size: 829 bytes
Desc: not available
URL: <http://lists.llvm.org/pipermail/cfe-dev/attachments/20090619/fe9abee4/attachment-0001.obj>