This is an archive of the discontinued LLVM Phabricator instance.

[C++20][Modules] Update handling of implicit inlines [P1779R3]
ClosedPublic

Authored by iains on Jul 3 2022, 7:04 AM.

Details

Summary

This provides updates to
[class.mfct]:
Pre C++20 [class.mfct]p2:

A member function may be defined (8.4) in its class definition, in
which case it is an inline member function (7.1.2)

Post C++20 [class.mfct]p1:

If a member function is attached to the global module and is defined
in its class definition, it is inline.

and
[class.friend]:
Pre-C++20 [class.friend]p5

A function can be defined in a friend declaration of a
class . . . . Such a function is implicitly inline.

Post C++20 [class.friend]p7

Such a function is implicitly an inline function if it is attached
to the global module.

We add the output of implicit-inline to the TextNodeDumper, and amend
a couple of existing tests to account for this, plus add tests for the
cases covered above.

Diff Detail

Event Timeline

iains created this revision.Jul 3 2022, 7:04 AM
Herald added a project: Restricted Project. · View Herald TranscriptJul 3 2022, 7:04 AM
iains added a subscriber: Restricted Project.
iains published this revision for review.Jul 3 2022, 7:08 AM

the other provisions of P1779 are handled elsewhere:

  1. private module fragment (which is also under discussion in core) and
  2. implicit inline status for constexpr/consteval which has been implemented elsewhere (and is not modules-dependent).
Herald added a project: Restricted Project. · View Herald TranscriptJul 3 2022, 7:08 AM
Herald added a subscriber: cfe-commits. · View Herald Transcript
ChuanqiXu added inline comments.Jul 3 2022, 7:14 PM
clang/lib/AST/TextNodeDumper.cpp
1723–1725

Is this necessary to this revision?

clang/lib/Sema/SemaDecl.cpp
9434

If I remember correctly, when we run -std=c++2b, getLangOpts().CPlusPlus20 would be true too. So we could check getLangOpts().CPlusPlus20 only here.

9435–9437

nit: this is not required.

9724

ditto

iains marked 4 inline comments as done.Jul 3 2022, 11:37 PM
iains added inline comments.
clang/lib/AST/TextNodeDumper.cpp
1723–1725

yes, to test it

clang/lib/Sema/SemaDecl.cpp
9434

will double-check.

9435–9437

well, the logic is required ;) .. you have suggested a different way to write..

9724

likewise.

iains updated this revision to Diff 442029.Jul 4 2022, 2:03 AM
iains marked 4 inline comments as done.

rebased, factored code to address comments.

iains added inline comments.Jul 4 2022, 2:07 AM
clang/lib/Sema/SemaDecl.cpp
9435–9437

actually, I think you meant !NewFD->getOwningModule() || NewFD->getOwningModule()->isGlobalModule() but I've pulled the test out so that it's only done once.

It looks like the tests lack the cast that the methods are attached to the global module fragment.

clang/lib/Sema/SemaDecl.cpp
9435–9437

Yeah, thanks!

clang/test/AST/ast-dump-constant-expr.cpp
94

Maybe it is good to add the newline although it is not your fault.

iains added a comment.Jul 4 2022, 2:23 AM

It looks like the tests lack the cast that the methods are attached to the global module fragment.

(maybe I misunderstand what you are saying here)

bool ImplicitInlineCXX20 = !getLangOpts().CPlusPlus20 ||
                               !NewFD->getOwningModule() ||
                               NewFD->getOwningModule()->isGlobalModule();

We are in the global module if there is no owning module, or if the owning module is the GMF.
We always set this to true before C++20, matching the unconditional true default argument it the setImplicitlyInline() method.

It looks like the tests lack the cast that the methods are attached to the global module fragment.

(maybe I misunderstand what you are saying here)

bool ImplicitInlineCXX20 = !getLangOpts().CPlusPlus20 ||
                               !NewFD->getOwningModule() ||
                               NewFD->getOwningModule()->isGlobalModule();

We are in the global module if there is no owning module, or if the owning module is the GMF.
We always set this to true before C++20, matching the unconditional true default argument it the setImplicitlyInline() method.

I mean the tests instead of the implementation. It looks like there is no GMF in the tests.

iains updated this revision to Diff 442072.Jul 4 2022, 5:29 AM

updated testcases to fix a missing new line and add included header in a GMF.

iains marked an inline comment as done.Jul 4 2022, 5:30 AM

It looks like the tests lack the cast that the methods are attached to the global module fragment.

(maybe I misunderstand what you are saying here)

I mean the tests instead of the implementation. It looks like there is no GMF in the tests.

added.

ChuanqiXu accepted this revision.Jul 4 2022, 7:09 PM

LGTM.

This revision is now accepted and ready to land.Jul 4 2022, 7:09 PM
This revision was landed with ongoing or failed builds.Jul 9 2022, 8:07 AM
This revision was automatically updated to reflect the committed changes.
iains reopened this revision.Jul 15 2022, 6:44 AM

reopening to post the patch I plan to re-land.

This revision is now accepted and ready to land.Jul 15 2022, 6:44 AM
iains updated this revision to Diff 444965.Jul 15 2022, 6:45 AM

rebased, fixed interaction with clang modules.