r203978 - Call RequireCompleteType when performing ADL even if the type is already

[Richard Smith]

On Fri, Mar 14, 2014 at 5:36 PM, David Blaikie <dblaikie at gmail.com> wrote:

> On Fri, Mar 14, 2014 at 3:07 PM, Richard Smith
> <richard-llvm at metafoo.co.uk> wrote:
> > Author: rsmith
> > Date: Fri Mar 14 17:07:27 2014
> > New Revision: 203978
> >
> > URL: http://llvm.org/viewvc/llvm-project?rev=203978&view=rev
> > Log:
> > Call RequireCompleteType when performing ADL even if the type is already
> > complete.
>
> I'm confused by this - looking at the debug info test cases you
> changed it seems odd that the type is required to be complete in those
> cases, given that the code still compiles if I replace the definition
> with a declaration in those files.


RequireCompleteType is being called here because the semantics of the
program could be affected by the completeness of the type -- in this case,
that's because the class is an associated class of the argument-dependent
lookup, so we look inside the definition of the class for any friends it
might have.


> > We hook into this check from a couple of other places (modules,
> > debug info) so it's not OK to elide the check if the type was already
> > complete.
>
> It might be helpful to have some modules test cases to cover this
> functionality - we might want to try to find some ways to avoid this
> for debug info... (though I'm not holding my breath for dealing with
> this any time soon)


You could possibly check whether the RequireCompleteType call is provided
with a diagnostic, but even that is probably a bit unreliable.


> >
> > Modified:
> >     cfe/trunk/lib/Sema/SemaLookup.cpp
> >     cfe/trunk/test/CodeGenCXX/debug-info-class-limited.cpp
> >     cfe/trunk/test/CodeGenCXX/debug-info-limited.cpp
> >
> > Modified: cfe/trunk/lib/Sema/SemaLookup.cpp
> > URL:
> http://llvm.org/viewvc/llvm-project/cfe/trunk/lib/Sema/SemaLookup.cpp?rev=203978&r1=203977&r2=203978&view=diff
> >
> ==============================================================================
> > --- cfe/trunk/lib/Sema/SemaLookup.cpp (original)
> > +++ cfe/trunk/lib/Sema/SemaLookup.cpp Fri Mar 14 17:07:27 2014
> > @@ -2027,6 +2027,10 @@ addAssociatedClassesAndNamespaces(Associ
> >
> >    // Add the class itself. If we've already seen this class, we don't
> >    // need to visit base classes.
> > +  //
> > +  // FIXME: That's not correct, we may have added this class only
> because it
> > +  // was the enclosing class of another class, and in that case we
> won't have
> > +  // added its base classes yet.
> >    if (!Result.Classes.insert(Class))
> >      return;
> >
> > @@ -2053,12 +2057,8 @@ addAssociatedClassesAndNamespaces(Associ
> >    }
> >
> >    // Only recurse into base classes for complete types.
> > -  if (!Class->hasDefinition()) {
> > -    QualType type = Result.S.Context.getTypeDeclType(Class);
> > -    if (Result.S.RequireCompleteType(Result.InstantiationLoc, type,
> > -                                     /*no diagnostic*/ 0))
> > -      return;
> > -  }
> > +  if (!Class->hasDefinition())
> > +    return;
> >
> >    // Add direct and indirect base classes along with their associated
> >    // namespaces.
> > @@ -2150,6 +2150,8 @@ addAssociatedClassesAndNamespaces(Associ
> >      //        classes. Its associated namespaces are the namespaces in
> >      //        which its associated classes are defined.
> >      case Type::Record: {
> > +      Result.S.RequireCompleteType(Result.InstantiationLoc, QualType(T,
> 0),
> > +                                   /*no diagnostic*/ 0);
> >        CXXRecordDecl *Class
> >          = cast<CXXRecordDecl>(cast<RecordType>(T)->getDecl());
> >        addAssociatedClassesAndNamespaces(Result, Class);
> >
> > Modified: cfe/trunk/test/CodeGenCXX/debug-info-class-limited.cpp
> > URL:
> http://llvm.org/viewvc/llvm-project/cfe/trunk/test/CodeGenCXX/debug-info-class-limited.cpp?rev=203978&r1=203977&r2=203978&view=diff
> >
> ==============================================================================
> > --- cfe/trunk/test/CodeGenCXX/debug-info-class-limited.cpp (original)
> > +++ cfe/trunk/test/CodeGenCXX/debug-info-class-limited.cpp Fri Mar 14
> 17:07:27 2014
> > @@ -24,7 +24,7 @@ foo *bar(foo *a) {
> >  }
> >
> >  namespace test1 {
> > -// CHECK-DAG: [ DW_TAG_structure_type ] [foo] [line [[@LINE+1]], {{.*}}
> [decl]
> > +// CHECK-DAG: [ DW_TAG_structure_type ] [foo] [line [[@LINE+1]], {{.*}}
> [def]
> >  struct foo {
> >  };
> >
> >
> > Modified: cfe/trunk/test/CodeGenCXX/debug-info-limited.cpp
> > URL:
> http://llvm.org/viewvc/llvm-project/cfe/trunk/test/CodeGenCXX/debug-info-limited.cpp?rev=203978&r1=203977&r2=203978&view=diff
> >
> ==============================================================================
> > --- cfe/trunk/test/CodeGenCXX/debug-info-limited.cpp (original)
> > +++ cfe/trunk/test/CodeGenCXX/debug-info-limited.cpp Fri Mar 14 17:07:27
> 2014
> > @@ -11,8 +11,7 @@ A *foo (A* x) {
> >    return a;
> >  }
> >
> > -// Verify that we're not emitting a full definition of B in limit debug
> mode.
> > -// CHECK: ; [ DW_TAG_class_type ] [B] {{.*}} [decl]
> > +// CHECK: ; [ DW_TAG_class_type ] [B] {{.*}} [def]
> >
> >  class B {
> >  public:
> >
> >
> > _______________________________________________
> > cfe-commits mailing list
> > cfe-commits at cs.uiuc.edu
> > http://lists.cs.uiuc.edu/mailman/listinfo/cfe-commits
>
-------------- next part --------------
An HTML attachment was scrubbed...
URL: <http://lists.llvm.org/pipermail/cfe-commits/attachments/20140314/a5152fd3/attachment.html>