[PATCH] D141908: [C++20][Modules] Handle defaulted and deleted functions in header units.
Iain Sandoe via Phabricator via cfe-commits
cfe-commits at lists.llvm.org
Wed Jan 18 01:56:58 PST 2023
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
More information about the cfe-commits
mailing list