Page MenuHomePhabricator

[Sema] Pop function scope when instantiating a func with skipped body
ClosedPublic

Authored by ilya-biryukov on Mar 13 2018, 9:56 AM.

Details

Summary

By calling ActOnFinishFunctionBody(). Previously we were only calling
ActOnSkippedFunctionBody, which didn't pop the function scope.
This causes a crash when running on our internal code. No test-case,
though, since I couldn't come up with a small example in reasonable
time.

The bug was introduced in r321174.

Diff Detail

Repository
rC Clang

Event Timeline

ilya-biryukov created this revision.Mar 13 2018, 9:56 AM
sammccall accepted this revision.Mar 14 2018, 2:07 AM
sammccall added inline comments.
lib/Sema/SemaTemplateInstantiateDecl.cpp
3943

(hmm, not sure whether it's better to duplicate or not-duplicate this comment. up to you)

This revision is now accepted and ready to land.Mar 14 2018, 2:07 AM
aaron.ballman requested changes to this revision.Mar 14 2018, 4:49 AM
aaron.ballman added a subscriber: aaron.ballman.

Test case for this change?

lib/Sema/SemaTemplateInstantiateDecl.cpp
3945

Rather than having two paths that call the same function, why not hoist the call out to the common path?

3962–3963

This change looks unrelated to the patch.

This revision now requires changes to proceed.Mar 14 2018, 4:49 AM

I also came up with a test case, but it breaks because of invalid errors when function bodies are skipped. I'll make sure to include it along with a fix for the errors in a follow-up patch.

lib/Sema/SemaTemplateInstantiateDecl.cpp
3943

It looks relevant to both sites, so I'd probably keep it. It adds some noise, but it's better than having a chance of confusing someone by not including it.

ilya-biryukov marked 4 inline comments as done.
  • Addressed review comments
aaron.ballman accepted this revision.Mar 14 2018, 5:25 AM

LGTM, thank you for the explanation about the tests.

This revision is now accepted and ready to land.Mar 14 2018, 5:25 AM

@aaron.ballman , here's a test-case I came up with. I'll fix the other issue (invalid error, that forces the test to fail) and submit it for review along with the fix for the error. Does that SG?

template <class T>
auto make_func() {
  struct impl {
    impl* func() {
      // body of this function is skipped during completion.
      // but will be instantiated anyway.
      return this;
    }
  };

  return impl();
}

void foo() {
  []() {
    make_func<int>();    // currently also produces invalid error `function with auto-deduced type 'make_func<int>' is used before defined
    m
    // RUN: %clang_cc1 -fsyntax-only -code-completion-at=%s:17:6 %s -o - | FileCheck %s
    // CHECK: make_func : [#auto#]make_func<<#class T#>>()

  };
}
lib/Sema/SemaTemplateInstantiateDecl.cpp
3962–3963

It was a formatting change.

@aaron.ballman , here's a test-case I came up with. I'll fix the other issue (invalid error, that forces the test to fail) and submit it for review along with the fix for the error. Does that SG?

Yes, that sounds good to me. Thank you!

This revision was automatically updated to reflect the committed changes.