This is an archive of the discontinued LLVM Phabricator instance.

[Sema] Don't emit undefined-internal warnings for CXXDeductionGuideDecls
ClosedPublic

Authored by cynecx on Jun 8 2017, 3:58 PM.

Details

Reviewers
rsmith
bebuch
Summary

Clang emits an undefined-internal warning for the following code:

template <typename T>
struct TemplDObj {
    explicit TemplDObj(T func) noexcept {}
};

auto test = TemplDObj([] {});

Warning:

<source>:3:14: warning: function '<deduction guide for TemplDObj><(lambda at example.cpp:6:23)>' has internal linkage but is not defined [-Wundefined-internal]
    explicit TemplDObj(T func) noexcept {}
             ^
<source>:6:13: note: used here
auto test = TemplDObj([] {});
            ^

It doesn't seem right to emit such warning for CXXDeductionGuideDecl.

This patch basically skips over CXXDeductionGuideDecl in Sema::getUndefinedButUsed.

Event Timeline

cynecx created this revision.Jun 8 2017, 3:58 PM
cynecx added a comment.Jun 9 2017, 4:30 PM

Does this require a test? Ping.

Gentle ping.

rsmith edited edge metadata.Jun 12 2017, 12:06 PM

Generally, all behavioural changes should have accompanying tests. The code change itself looks good, thanks.

cynecx updated this revision to Diff 102260.Jun 12 2017, 4:31 PM
  • [Sema] Fix 'has internal linkage but is not defined' for CXXDeductionGuideDecl
  • Add new test

Gentle ping for review.

Another gentle ping :)

rsmith added inline comments.Jun 15 2017, 7:54 AM
test/SemaCXX/cxx1z-class-template-argument-deduction.cpp
2

Why do we need to split the test up like this? If there's a reason, the test should include a comment explaining it.

cynecx updated this revision to Diff 102694.Jun 15 2017, 11:21 AM
  • Add comment in test
cynecx marked an inline comment as done.Jun 15 2017, 2:24 PM
cynecx added inline comments.
test/SemaCXX/cxx1z-class-template-argument-deduction.cpp
2

Is this better?

bebuch accepted this revision.Jun 16 2017, 11:26 AM
bebuch added a subscriber: bebuch.

Perfect. Tested on my own project with TOT clang.

$clang --version
clang version 5.0.0 (http://llvm.org/git/clang.git 6adac51dabb51771aade9b29f059f562b7f457c3) (http://llvm.org/git/llvm.git 470c6959b7834acd5191390007439eaf95b087a6)
Target: x86_64-unknown-linux-gnu
Thread model: posix
This revision is now accepted and ready to land.Jun 16 2017, 11:26 AM
cynecx marked an inline comment as done.Jun 20 2017, 8:01 AM

Thanks. So when can this be committed?

rsmith added inline comments.Jul 7 2017, 12:43 PM
test/SemaCXX/cxx1z-class-template-argument-deduction.cpp
2

No -- the comment explains what you did, but not why. Why can't you remove the #ifdefs and one of the RUN: lines?

cynecx updated this revision to Diff 105679.Jul 7 2017, 12:56 PM
  • [Sema] Fix 'has internal linkage but is not defined' for CXXDeductionGuideDecl
  • Add new test
  • Add comment in test
  • Fix comment
test/SemaCXX/cxx1z-class-template-argument-deduction.cpp
2

Is this better?

Will this fix be part of clang-5? It would be very annoying to have this fixed, but not to use it. Of course I understand that is is also important to have a good unit test.

Sorry for the delay. Let's get this in for Clang 5.0. Thank you!

test/SemaCXX/cxx1z-class-template-argument-deduction.cpp
2

I think you have a misunderstanding about how -verify works. A -cc1 execution with -verify will fail unless there is exactly one expected-blah line for each produced diagnostic. If a block of code is expected to produce no diagnostics, you can just add that to the file with no expected-blah lines, and the test will fail if it produces any diagnostics. expected-no-diagnostics is just a marker that says "I intentionally didn't put any expected-blah lines in this file.

I think the reason you need to split the test is actually that we don't produce "undefined internal" warnings if we've produced any errors. I'm going to update the comment to say that and then check this in.

rsmith closed this revision.Aug 3 2017, 12:26 PM

Landed in r309975.

cynecx added inline comments.Aug 3 2017, 1:31 PM
test/SemaCXX/cxx1z-class-template-argument-deduction.cpp
2

Oh, I though the expected-no-diagnostics actually had a function.

Thanks for clarifying and landing this patch :).