iains added a comment. 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?
================ 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: > 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) Repository: rG LLVM Github Monorepo CHANGES SINCE LAST ACTION https://reviews.llvm.org/D142704/new/ https://reviews.llvm.org/D142704 _______________________________________________ cfe-commits mailing list cfe-commits@lists.llvm.org https://lists.llvm.org/cgi-bin/mailman/listinfo/cfe-commits