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
_______________________________________________
cfe-commits mailing list
cfe-commits@lists.llvm.org
https://lists.llvm.org/cgi-bin/mailman/listinfo/cfe-commits
