[PATCH] D142704: [C++20][Modules] Handle template declarations in header units.

[Chuanqi Xu via Phabricator via cfe-commits]

ChuanqiXu added a comment.

In D142704#4092724 <https://reviews.llvm.org/D142704#4092724>, @iains wrote:

> I think we need to find a way to proceed - because this causes a regression on the llvm-16 branch, and that should be resolved soon, if possible.
> What is your suggestion for a way forward?

Yeah, this is a pretty severe problem. Luckily clang16 is going to be released in early March. So we have roughly one month to solve this problem. Even if we failed to solve this in the last minute, I think it is not too bad to revert https://github.com/llvm/llvm-project/commit/335668b116439d13c7555616e126acdc608ce59e.



================
Comment at: clang/lib/Sema/SemaDecl.cpp:15265
       FD->getFormalLinkage() == Linkage::ExternalLinkage &&
-      !FD->isInvalidDecl() && BodyKind != FnBodyKind::Delete &&
+      !FD->isInvalidDecl() && !IsFnTemplate && BodyKind != FnBodyKind::Delete &&
       BodyKind != FnBodyKind::Default && !FD->isInlined()) {
----------------
iains wrote:
> iains wrote:
> > ChuanqiXu wrote:
> > > iains wrote:
> > > > rsmith wrote:
> > > > > Would it make sense to use `!isa<FunctionTemplateDecl>(D)` here instead of adding `IsFnTemplate`?
> > > > > Would it make sense to use `!isa<FunctionTemplateDecl>(D)` here instead of adding `IsFnTemplate`?
> > > > 
> > > > I have changed this to use FD->isTemplated() to match the changes for VarDecls - where the template decl is not available.  Would it be better to use the isa<>() ?
> > > > 
> > > It looks not bad to me to use `isTemplated ()`. And it looks like `!FD->isTemplateInstantiation()` is not tested? And it looks a little bit odd since the instantiated template should be implicitly inline.
> > > It looks not bad to me to use `isTemplated ()`. 
> > 
> > Hmm. now I am not sure what you prefer; in a previous review there was an objection to not using the provided function in the decl class.  
> > 
> > What would be your suggestion here?
> > 
> > (we cannot use isa<VarTemplateDecl> when testing the variables case because the caller pulls out the templated decl before we get to test it) - so it is more consistent (IMO) to use the same interface in both tests.
> > 
> > >And it looks like `!FD->isTemplateInstantiation()` is not tested?
> > 
> > Please could you expand a bit on what you mean here?
> >  (the test is clearly required, in practice)
> > 
> > > And it looks a little bit odd since the instantiated template should be implicitly inline.
> > 
> > Did you see @dlaikie's comment on this topic above?
> > 
> > 
> 
> > Did you see @dlaikie's comment on this topic above?
> 
> sorry @dblaikie 's comment is in the PR (https://github.com/llvm/llvm-project/issues/60079#issuecomment-1406856501)
> What would be your suggestion here?

I prefer `isTemplated ()` here. If I get things correct,  `isTemplated ()` will cover the case that a non-template function decl inside a function template. And `isa<FunctionTemplateDecl>(D)` wouldn't handle the case. According to my understanding problem, we should avoid all the dependent cases.

> Please could you expand a bit on what you mean here?

I mean I didn't find function template instantiation in tests of this revision page.

> sorry @dblaikie 's comment is in the PR (https://github.com/llvm/llvm-project/issues/60079#issuecomment-1406856501)

oh, this is because the inconsistent use of `inline` in different places.. this may be the price we need to pay.


Repository:
  rG LLVM Github Monorepo

CHANGES SINCE LAST ACTION
  https://reviews.llvm.org/D142704/new/

https://reviews.llvm.org/D142704