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

[Iain Sandoe via Phabricator via cfe-commits]

iains added a comment.

In D141908#4061451 <https://reviews.llvm.org/D141908#4061451>, @ChuanqiXu wrote:

>> 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 : )

(I added the FIXME) Have a good holiday!



================
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() &&
----------------
ChuanqiXu wrote:
> 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)

> > 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. 

I re-checked (to remind myself) .. essentially that state is set by SetFunctionBodyKind() which is called much later.  I still think we would most likely end up with having to place the diagnostic in multiple places.  However, I added the FIXME,

> And it looks not impossible to refactor it. (I'm not asking you to do the change)

such a refactoring looks non-trivial and certainly not suitable for a few days before a branch ...



Repository:
  rG LLVM Github Monorepo

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

https://reviews.llvm.org/D141908