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