[PATCH] D141908: [C++20][Modules] Handle defaulted and deleted functions in header units.

[Chuanqi Xu via Phabricator via cfe-commits]

ChuanqiXu added a comment.

> Well.. we have time for another iteration,

I am going to take a vacation for the Chinese New Year since tomorrow to February. So I am a little bit hurried : )



================
Comment at: clang/lib/Sema/SemaDecl.cpp:15258
+  // units.  Deleted and Defaulted functions are implicitly inline (but the
+  // inline state is not set at this point, so check the BodyKind explicitly).
   if (getLangOpts().CPlusPlusModules && currentModuleIsHeaderUnit() &&
----------------
iains wrote:
> ChuanqiXu wrote:
> > iains wrote:
> > > ChuanqiXu wrote:
> > > > I prefer to add a FIXME here to say that we need to find a better place for the check to eliminate the unnecessary check for `BodyKind `. The current check for `BodyKind` looks a little bit hacky to me.
> > > When the patch was originally done, this was found to be a good place to do the check (i.e. less duplication of testing and to avoid duplication of diagnostics) so I do not think I agree that there is a FIXME to move it.
> > > 
> > > BodyKind is already used elsewhere in this function for similar purposes - it does not look hacky to me.
> > It looks hacky to me since we shouldn't care if it is deleted or defaulted here and it should be enough to check `FD->isInlied()`.  And I don't see similar usage of `BodyKind ` in this function.
> > It looks hacky to me since we shouldn't care if it is deleted or defaulted here and it should be enough to check `FD->isInlied()`. 
> 
> that means checking much later and in muliple places, I think - but if you want to make a follow-on patch, I will be happy to review.
> 
> > And I don't see similar usage of `BodyKind ` in this function.
> 
> line 15208?
> 
> 
> line 15208?

line 15208 checks the wording `unless the function is deleted (C++ specifc, C++ [dcl.fct.def.general]p2)`. So it is used to check if this is a delete function, which looks fine. I mean it would be best if we can check `FD->isInlined()` only.

> that means checking much later and in muliple places, I think - but if you want to make a follow-on patch, I will be happy to review.

I think I won't work on this. But it would be meaningful for future developers. And it looks not impossible to refactor it. (I'm not asking you to do the change)


================
Comment at: clang/lib/Sema/SemaDecl.cpp:15261
+      FD->getFormalLinkage() == Linkage::ExternalLinkage &&
+      !FD->isInvalidDecl() && BodyKind == FnBodyKind::Other &&
+      !FD->isInlined()) {
----------------
iains wrote:
> ChuanqiXu wrote:
> > iains wrote:
> > > ChuanqiXu wrote:
> > > > It looks like we need to check `FD->isThisDeclarationADefinition()` too.
> > > > 
> > > > And personally, I prefer to check BodyKind explicitly. Otherwise the readers need to checkout the definition of `FnBodyKind` to understand the code. 
> > > > It looks like we need to check `FD->isThisDeclarationADefinition()` too.
> > > 
> > > This is an unnecessary test, it will always return true at this point.
> > > 
> > > > And personally, I prefer to check BodyKind explicitly. Otherwise the readers need to checkout the definition of `FnBodyKind` to understand the code. 
> > > 
> > > You prefer two tests  instead of one?
> > > OK, I guess
> > > This is an unnecessary test, it will always return true at this point.
> > 
> > Oh, I found it now. It may be better to have an assertion `assert(FD->isThisDeclarationADefinition())`.
> yeah, I was thinking maybe to do that (it is kind of documenting that it is always true - perhaps a comment would be better?)
> 
I always prefer assertion than comments since  some people may not like to read the comments. And the assertion may be useful. For example, if someday we want to move the codes to another place but the FD is not always a definition. Then the assertion can save us a lot of time.


Repository:
  rG LLVM Github Monorepo

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

https://reviews.llvm.org/D141908