This is an archive of the discontinued LLVM Phabricator instance.

[Sema] Pass CombineWithOuterScope = true to constructor of LocalInstantiationScope
ClosedPublic

Authored by ahatanak on Aug 2 2016, 4:39 PM.

Details

Summary

This fixes PR28795.

https://llvm.org/bugs/show_bug.cgi?id=28795

Sema wasn't replacing DependentScopeDeclRefExpr with DeclRefExpr during template instantiation of the default argument of the lambda function because it failed to find the instantiated enum "foo" in LocalInstantiationScope. This patch fixes the bug by allowing it to search the outer scope (function func).

Diff Detail

Repository
rL LLVM

Event Timeline

ahatanak updated this revision to Diff 66591.Aug 2 2016, 4:39 PM
ahatanak retitled this revision from to [Sema] Pass CombineWithOuterScope = true to constructor of LocalInstantiationScope.
ahatanak updated this object.
ahatanak added reviewers: rsmith, sepavloff.
ahatanak added a subscriber: cfe-commits.
sepavloff added inline comments.Aug 3 2016, 11:01 AM
test/SemaTemplate/default-expr-arguments-3.cpp
20 ↗(On Diff #66591)

Please add the following test to the patch:

template<typename T> void f1() {
  enum class foo { a, b };
  struct S {
    int g1(foo n = foo::a);
  };
}

template void f1<int>();

It must also compile correctly.

OK, I'll add the test.

When I apply this patch and compile your test, I see DependentScopeDeclRefExpr gets converted to DeclRefExpr when the function is instantiated. This doesn't happen without this patch, but somehow clang still manages to emit the same code.

Also, Erik Pilkington pointed out that clang still crashes when it compiles the following code. I'm trying to figure out why that happens.

// Template variable case:
enum class foo { a, b };
template <class T> auto a = [](foo f = foo::a) { return f; }();

auto foo1() {
  return a<int>;
}

// Template struct case:
template <class T> struct func {
  void bar() {
    enum class foo { a, b };
    [](foo f = foo::a) { return f; }();
  }
};

template struct func<int>;

In each of these cases, "foo::a" is a non-dependent type, so it seems that clang shouldn't use DependentScopeDeclRefExpr or CXXDependentScopeMemberExpr in the AST when the template definitions are parsed.

ahatanak updated this revision to Diff 67623.Aug 10 2016, 4:06 PM

Handle the case in which the enum is declared inside a member function and add test cases.

The variable template test case still crashes. It looks like it crashes whenever the lambda expression takes a default argument:

template <class T> auto a = [](int f = 7) { return f; }();

auto foo1() {

return a<int>;

}

ahatanak updated this revision to Diff 68078.Aug 15 2016, 2:27 PM

Fix Sema::getTemplateInstantiationArgs to return the template instantiation args when variable templates are being instantiated.

sepavloff added inline comments.Sep 6 2016, 11:47 PM
include/clang/Sema/Sema.h
6619 ↗(On Diff #68078)

I would propose to use something like 'getVarTemplateArguments' to make the name more similar to other methods.

lib/Sema/SemaTemplateInstantiate.cpp
193–205 ↗(On Diff #68078)

The code does not seem to be the best solution. It assumes that if instantiation of a variable is requested, its arguments define all parameters needed to instantiate code. It is not so for the code:

template<typename T2>
struct C1 {
  template<typename T>
  static int v1;
};

template<typename T2>
template<typename T>
int C1<T2>::v1 = [](int a = T2(1)) { return a; }();

int main(int argc, char *argv[]) {
  (void) C1<long*>::v1<int>;
  return 0;
}

This problem is caused by different nature of variable templates. When instantiating a function or a class all instantiated entities go to the contexts defined by the class or the function respectively. A variable does not represent a declaration context and its initializer and all declarations created during the instantiation go to enclosing context. So logic in getTemplateInstantiationArgs does not work.
May be we should extend the logic of getTemplateInstantiationArgs so that it supports variable template and still remains recursive?

ahatanak updated this revision to Diff 72356.Sep 23 2016, 2:39 PM

I agree that extending the logic of getTemplateInstantiationArgs seems like a better approach. I changed Sema::getTemplateInstantiationArgs to search for the template arguments twice, first for the initializer's template arguments and then for the VarTemplateSpecializationDecl. This should enable handling cases where a variable template is declared inside a class/struct.

I wonder whether VarTemplateSpecializationDecl can be made a subclass of DeclContext and have it contain the lambda class of the initializer. I think that will simplify the logic of getTemplateInstantiationArgs.

sepavloff edited edge metadata.Oct 5 2016, 11:29 AM

IIUC, the problem is observed because Sema::getTemplateInstantiationArgs does not handle the case of variable templates properly. Classes and functions are declaration contexts and implementation of the aforementioned function (and probably others) relies on this fact. Variable does not represents a context, and this causes errors like this. We cannot make VarTemplateSpecializationDecl a subclass of DeclContext because the latter not only serves as a host for other declarations but also supports name lookup. None is pertinent to the case of variable specialization.

I think, logic of getTemplateInstantiationArgs should be changed. The new implementation could inspect current instantiation (Sema::ActiveTemplateInstantiations) to check if it is an instantiation of a variable template. This could eliminate need of VarTemplateSpec and VarTemplateSpecializationRAII. Such solution looks more flexible as variable initializer may contain references to other variable instantiations, so single value of VarTemplateSpec is not sufficient to track instantiations.

Do you think we can transform the Exprs for the uninstantiated default arguments in TreeTransform<Derived>::TransformLambdaExpr? I guess it would be much simpler than trying to make getTemplateInstantiationArgs return the correct template arguments.

ahatanak updated this revision to Diff 77691.Nov 11 2016, 3:59 PM
ahatanak edited edge metadata.
  • Transform uninstantiated default arguments Exprs in TreeTransform<Derived>::TransformLambdaExpr.
  • In default-expr-arguments-3.cpp, check that there are no crashes or diagnostics emitted instead of printing the AST.
ahatanak updated this revision to Diff 81171.Dec 12 2016, 6:12 PM

Rebase and ping.

sepavloff accepted this revision.Dec 13 2016, 9:27 PM
sepavloff edited edge metadata.

Looks good. Thank you!

This revision is now accepted and ready to land.Dec 13 2016, 9:27 PM
This revision was automatically updated to reflect the committed changes.