[cfe-commits] r168521 - in /cfe/trunk: include/clang/AST/ExprCXX.h lib/Sema/SemaOverload.cpp test/SemaTemplate/default-expr-arguments.cpp

[Benjamin Kramer]

On 23.11.2012, at 23:17, Richard Smith <richard at metafoo.co.uk> wrote:

> On Fri, Nov 23, 2012 at 11:47 AM, Benjamin Kramer <benny.kra at gmail.com> wrote:
>> 
>> On 23.11.2012, at 18:04, Benjamin Kramer <benny.kra at googlemail.com> wrote:
>> 
>>> Author: d0k
>>> Date: Fri Nov 23 11:04:52 2012
>>> New Revision: 168521
>>> 
>>> URL: http://llvm.org/viewvc/llvm-project?rev=168521&view=rev
>>> Log:
>>> Sema: Provide a valid source location when instantiating templates based on a CXXDefaultArgExpr.
>>> 
>>> Fixes PR13758.
>>> 
>>> Modified:
>>>   cfe/trunk/include/clang/AST/ExprCXX.h
>>>   cfe/trunk/lib/Sema/SemaOverload.cpp
>>>   cfe/trunk/test/SemaTemplate/default-expr-arguments.cpp
>>> 
>>> Modified: cfe/trunk/include/clang/AST/ExprCXX.h
>>> URL: http://llvm.org/viewvc/llvm-project/cfe/trunk/include/clang/AST/ExprCXX.h?rev=168521&r1=168520&r2=168521&view=diff
>>> ==============================================================================
>>> --- cfe/trunk/include/clang/AST/ExprCXX.h (original)
>>> +++ cfe/trunk/include/clang/AST/ExprCXX.h Fri Nov 23 11:04:52 2012
>>> @@ -795,6 +795,8 @@
>>>    return SourceRange();
>>>  }
>>> 
>>> +  SourceLocation getExprLoc() const LLVM_READONLY { return Loc; }
>>> +
>>>  static bool classof(const Stmt *T) {
>>>    return T->getStmtClass() == CXXDefaultArgExprClass;
>>>  }
>>> 
>>> Modified: cfe/trunk/lib/Sema/SemaOverload.cpp
>>> URL: http://llvm.org/viewvc/llvm-project/cfe/trunk/lib/Sema/SemaOverload.cpp?rev=168521&r1=168520&r2=168521&view=diff
>>> ==============================================================================
>>> --- cfe/trunk/lib/Sema/SemaOverload.cpp (original)
>>> +++ cfe/trunk/lib/Sema/SemaOverload.cpp Fri Nov 23 11:04:52 2012
>>> @@ -2980,7 +2980,7 @@
>>>         S.IsDerivedFrom(From->getType(), ToType)))
>>>      ConstructorsOnly = true;
>>> 
>>> -    S.RequireCompleteType(From->getLocStart(), ToType, 0);
>>> +    S.RequireCompleteType(From->getExprLoc(), ToType, 0);
> 
> Hmm, does getLocStart on a CXXDefaultArgExpr not work?

CXXDefaultArgExprs don't exist physically in the code, so getLocStart always returns an invalid SourceLocation.

>>>    // RequireCompleteType may have returned true due to some invalid decl
>>>    // during template instantiation, but ToType may be complete enough now
>>>    // to try to recover.
>>> 
>>> Modified: cfe/trunk/test/SemaTemplate/default-expr-arguments.cpp
>>> URL: http://llvm.org/viewvc/llvm-project/cfe/trunk/test/SemaTemplate/default-expr-arguments.cpp?rev=168521&r1=168520&r2=168521&view=diff
>>> ==============================================================================
>>> --- cfe/trunk/test/SemaTemplate/default-expr-arguments.cpp (original)
>>> +++ cfe/trunk/test/SemaTemplate/default-expr-arguments.cpp Fri Nov 23 11:04:52 2012
>>> @@ -303,3 +303,21 @@
>>>  {
>>>  }
>>> }
>>> +
>>> +namespace PR13758 {
>>> +  template <typename T> struct move_from {
>>> +    T invalid; // expected-error {{field has incomplete type 'void'}}
>>> +  };
>>> +  template <class K>
>>> +  struct unordered_map {
>>> +    explicit unordered_map(int n = 42);
>>> +    unordered_map(move_from<K> other);
>>> +  };
>>> +  template<typename T>
>>> +  void StripedHashTable() {
>>> +    new unordered_map<void>(); // expected-note {{in instantiation of template class 'PR13758::move_from<void>' requested here}}
>>> +  }
>>> +  void tt() {
>>> +    StripedHashTable<int>(); // expected-note {{in instantiation of function template specialization 'PR13758::StripedHashTable<int>' requested here}}
>>> +  }
>>> +}
>> 
>> Looking closer at the test case, I don't see why it instantiates move_from<void> at that point. GCC and EDG accept the code, maybe there is another clang bug here. Any ideas?
> 
> Yes, we shouldn't be instantiating move_from<void> here. llvm.org/demo
> accepts this code, so it looks like this is a regression.
> 
> Looking just at what you needed to change in your patch, my wild guess
> is that we might be trying to initialize an unordered_map object from
> the default argument expression '42', rather than trying to initialize
> the 'n' parameter of the explicit constructor from it. That would
> explain why we instantiate move_from<K>: we'd be trying to use the
> second constructor to convert '42' to an unordered_map, but would be a
> very surprising bug to have...

Ok, I filed http://llvm.org/bugs/show_bug.cgi?id=14428 to track this.

- Ben