[C++20][Modules] NFC Reworked handling of inline for functions defined in class #109470

tomasz-kaminski-sonarsource · 2024-09-20T20:25:39Z

Reworked handling of implicit inline marking for member and friend function defined in class.
Now, we handle it in an additive manner, i.e. if such in-class functions are inline implicitly by language rules,
we mark the as setImplicitInline, and perform no action otherwise.
As we never remove inline specifier, the implementation is orthogonal to other sources of inline
(like inline, constexpr, e.t.c), and we do not need to handle them specially.

Also included test for constexpr, consteval and global module cases.

llvmbot · 2024-09-20T20:26:17Z

@llvm/pr-subscribers-clang

Author: None (tomasz-kaminski-sonarsource)

Changes

This correct issue, when the functions declared as constexpr are consteval, are not considered to be inline (isInlined() is false) when defined inside the class attached to the named module:

export module mod;

struct Clazz {
  constexpr void f1() { } // non-inline
  constexpr void f2();
  friend constexpr void f3() {} // non-inline
};

constexpr void Clazz::f3() {} // inline

This conflicts with [decl.constexpr] p1:
> A function or static data member declared with the constexpr or consteval specifier on its first declaration is implicitly an inline function or variable ([dcl.inline]). If any declaration of a function or function template has a constexpr or consteval specifier, then all its declarations shall contain the same specifier.

This regression was introduced by https://github.com/SonarSource/llvm-project/commit/97af17c5, where the inline of such a function was accidentally removed. The corresponding wording in [class.friend] and p6 [class.mfct] p1 uses "if" and not "if and only if", thus not implying that these are the only cases where such functions are inline.

Full diff: https://github.com/llvm/llvm-project/pull/109470.diff

3 Files Affected:

(modified) clang/lib/Sema/SemaDecl.cpp (+7-7)
(modified) clang/test/CXX/class/class.friend/p7-cxx20.cpp (+33-5)
(modified) clang/test/CXX/class/class.mfct/p1-cxx20.cpp (+27-3)

diff --git a/clang/lib/Sema/SemaDecl.cpp b/clang/lib/Sema/SemaDecl.cpp
index de8805e15bc750..0ea99c43037e5e 100644
--- a/clang/lib/Sema/SemaDecl.cpp
+++ b/clang/lib/Sema/SemaDecl.cpp
@@ -9760,8 +9760,8 @@ Sema::ActOnFunctionDeclarator(Scope *S, Declarator &D, DeclContext *DC,
   if (getLangOpts().CPlusPlus) {
     // The rules for implicit inlines changed in C++20 for methods and friends
     // with an in-class definition (when such a definition is not attached to
-    // the global module).  User-specified 'inline' overrides this (set when
-    // the function decl is created above).
+    // the global module). This does not affect declarations, that are already
+    // inline, for example due being declared `inline` or `consteval`
     // FIXME: We need a better way to separate C++ standard and clang modules.
     bool ImplicitInlineCXX20 = !getLangOpts().CPlusPlusModules ||
                                NewFD->isConstexpr() || NewFD->isConsteval() ||
@@ -9772,14 +9772,14 @@ Sema::ActOnFunctionDeclarator(Scope *S, Declarator &D, DeclContext *DC,
     bool isVirtual = D.getDeclSpec().isVirtualSpecified();
     bool hasExplicit = D.getDeclSpec().hasExplicitSpecifier();
     isFriend = D.getDeclSpec().isFriendSpecified();
-    if (isFriend && !isInline && D.isFunctionDefinition()) {
+    if (ImplicitInlineCXX20 && isFriend && D.isFunctionDefinition()) {
       // 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.
-      NewFD->setImplicitlyInline(ImplicitInlineCXX20);
+      NewFD->setImplicitlyInline();
     }
 
     // If this is a method defined in an __interface, and is not a constructor
@@ -10083,15 +10083,15 @@ Sema::ActOnFunctionDeclarator(Scope *S, Declarator &D, DeclContext *DC,
       break;
     }
 
-    if (isa<CXXMethodDecl>(NewFD) && DC == CurContext &&
-        D.isFunctionDefinition() && !isInline) {
+    if (ImplicitInlineCXX20 && isa<CXXMethodDecl>(NewFD) && DC == CurContext &&
+        D.isFunctionDefinition()) {
       // 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.
-      NewFD->setImplicitlyInline(ImplicitInlineCXX20);
+      NewFD->setImplicitlyInline();
     }
 
     if (!isFriend && SC != SC_None) {
diff --git a/clang/test/CXX/class/class.friend/p7-cxx20.cpp b/clang/test/CXX/class/class.friend/p7-cxx20.cpp
index 8843d55910ea2d..0ce77b353c2499 100644
--- a/clang/test/CXX/class/class.friend/p7-cxx20.cpp
+++ b/clang/test/CXX/class/class.friend/p7-cxx20.cpp
@@ -46,14 +46,42 @@ module;
 export module M;
 
 class Z {
-  friend void z(){};
+  friend void z1(){};
 };
+
+class Inline {
+  friend inline void z2(){};
+};
+
+class Constexpr {
+  friend constexpr void z3(){};
+};
+
+class Consteval {
+  friend consteval void z4(){};
+};
+
 // CHECK-MOD: |-CXXRecordDecl {{.*}} <.{{/|\\\\?}}header.h:2:1, line:4:1> line:2:7 in M.<global> hidden class A definition
 // CHECK-MOD: | |-CXXRecordDecl {{.*}} <col:1, col:7> col:7 in M.<global> hidden implicit class A
 // CHECK-MOD-NEXT: | `-FriendDecl {{.*}} <line:3:3, col:19> col:15 in M.<global>
 // CHECK-MOD-NEXT: |   `-FunctionDecl {{.*}} parent {{.*}} <col:3, col:19> col:15 in M.<global> hidden friend_undeclared a 'void ()' implicit-inline
 
-// CHECK-MOD: `-CXXRecordDecl {{.*}} <module.cpp:6:1, line:8:1> line:6:7 in M hidden class Z{{( ReachableWhenImported)?}} definition
-// CHECK-MOD: |-CXXRecordDecl {{.*}} <col:1, col:7> col:7 in M hidden implicit class Z{{( ReachableWhenImported)?}}
-// CHECK-MOD-NEXT: `-FriendDecl {{.*}} <line:7:3, col:19> col:15 in M{{( ReachableWhenImported)?}}
-// CHECK-MOD-NEXT: `-FunctionDecl {{.*}} parent {{.*}} <col:3, col:19> col:15 in M hidden friend_undeclared z 'void ()'{{( ReachableWhenImported)?}}
+// CHECK-MOD: |-CXXRecordDecl {{.*}} <module.cpp:6:1, line:8:1> line:6:7 in M hidden class Z{{( ReachableWhenImported)?}} definition
+// CHECK-MOD: | |-CXXRecordDecl {{.*}} <col:1, col:7> col:7 in M hidden implicit class Z{{( ReachableWhenImported)?}}
+// CHECK-MOD-NEXT: | `-FriendDecl {{.*}} <line:7:3, col:20> col:15 in M{{( ReachableWhenImported)?}}
+// CHECK-MOD-NEXT: |   `-FunctionDecl {{.*}} parent {{.*}} <col:3, col:20> col:15 in M hidden friend_undeclared z1 'void ()'{{( ReachableWhenImported)?}}
+
+// CHECK-MOD: |-CXXRecordDecl {{.*}} <line:10:1, line:12:1> line:10:7 in M hidden class Inline{{( ReachableWhenImported)?}} definition
+// CHECK-MOD: | |-CXXRecordDecl {{.*}} <col:1, col:7> col:7 in M hidden implicit class Inline{{( ReachableWhenImported)?}}
+// CHECK-MOD-NEXT: | `-FriendDecl {{.*}} <line:11:3, col:27> col:22 in M{{( ReachableWhenImported)?}}
+// CHECK-MOD-NEXT: |   `-FunctionDecl {{.*}} parent {{.*}} <col:3, col:27> col:22 in M hidden friend_undeclared z2 'void ()'{{( ReachableWhenImported)?}} inline
+
+// CHECK-MOD: |-CXXRecordDecl {{.*}} <line:14:1, line:16:1> line:14:7 in M hidden class Constexpr{{( ReachableWhenImported)?}} definition
+// CHECK-MOD: | |-CXXRecordDecl {{.*}} <col:1, col:7> col:7 in M hidden implicit class Constexpr{{( ReachableWhenImported)?}}
+// CHECK-MOD-NEXT: | `-FriendDecl {{.*}} <line:15:3, col:30> col:25 in M{{( ReachableWhenImported)?}}
+// CHECK-MOD-NEXT: |   `-FunctionDecl {{.*}} parent {{.*}} <col:3, col:30> col:25 in M hidden constexpr friend_undeclared z3 'void ()'{{( ReachableWhenImported)?}} implicit-inline
+
+// CHECK-MOD: `-CXXRecordDecl {{.*}} <line:18:1, line:20:1> line:18:7 in M hidden class Consteval{{( ReachableWhenImported)?}} definition
+// CHECK-MOD: |-CXXRecordDecl {{.*}} <col:1, col:7> col:7 in M hidden implicit class Consteval{{( ReachableWhenImported)?}}
+// CHECK-MOD-NEXT: `-FriendDecl {{.*}} <line:19:3, col:30> col:25 in M{{( ReachableWhenImported)?}}
+// CHECK-MOD-NEXT: `-FunctionDecl {{.*}} parent {{.*}} <col:3, col:30> col:25 in M hidden consteval friend_undeclared z4 'void ()'{{( ReachableWhenImported)?}} implicit-inline
diff --git a/clang/test/CXX/class/class.mfct/p1-cxx20.cpp b/clang/test/CXX/class/class.mfct/p1-cxx20.cpp
index 5b24668d7b6617..9b990e13c09791 100644
--- a/clang/test/CXX/class/class.mfct/p1-cxx20.cpp
+++ b/clang/test/CXX/class/class.mfct/p1-cxx20.cpp
@@ -48,10 +48,34 @@ class Z {
   void z(){};
 };
 
+class Inline {
+  inline void z(){};
+};
+
+class Constexpr {
+  constexpr void z(){};
+};
+
+class Consteval {
+  consteval void z(){};
+};
+
 // CHECK-MOD: |-CXXRecordDecl {{.*}} <.{{/|\\\\?}}header.h:2:1, line:4:1> line:2:7 in M.<global> hidden class A definition
 // CHECK-MOD: | |-CXXRecordDecl {{.*}} <col:1, col:7> col:7 in M.<global> hidden implicit class A
 // CHECK-MOD-NEXT: | `-CXXMethodDecl {{.*}} <line:3:3, col:12> col:8 in M.<global> hidden a 'void ()' implicit-inline
 
-// CHECK-MOD: `-CXXRecordDecl {{.*}} <module.cpp:6:1, line:8:1> line:6:7 in M hidden class Z{{( ReachableWhenImported)?}} definition
-// CHECK-MOD: |-CXXRecordDecl {{.*}} <col:1, col:7> col:7 in M hidden implicit class Z{{( ReachableWhenImported)?}}
-// CHECK-MOD-NEXT: `-CXXMethodDecl {{.*}} <line:7:3, col:12> col:8 in M hidden z 'void ()'{{( ReachableWhenImported)?}}
+// CHECK-MOD: |-CXXRecordDecl {{.*}} <module.cpp:6:1, line:8:1> line:6:7 in M hidden class Z{{( ReachableWhenImported)?}} definition
+// CHECK-MOD: | |-CXXRecordDecl {{.*}} <col:1, col:7> col:7 in M hidden implicit class Z{{( ReachableWhenImported)?}}
+// CHECK-MOD-NEXT: | `-CXXMethodDecl {{.*}} <line:7:3, col:12> col:8 in M hidden z 'void ()'{{( ReachableWhenImported)?}}
+
+// CHECK-MOD: |-CXXRecordDecl {{.*}} <line:10:1, line:12:1> line:10:7 in M hidden class Inline{{( ReachableWhenImported)?}} definition
+// CHECK-MOD: | |-CXXRecordDecl {{.*}} <col:1, col:7> col:7 in M hidden implicit class Inline{{( ReachableWhenImported)?}}
+// CHECK-MOD-NEXT: | `-CXXMethodDecl {{.*}} <line:11:3, col:19> col:15 in M hidden z 'void ()'{{( ReachableWhenImported)?}} inline
+
+// CHECK-MOD: |-CXXRecordDecl {{.*}} <line:14:1, line:16:1> line:14:7 in M hidden class Constexpr{{( ReachableWhenImported)?}} definition
+// CHECK-MOD: | |-CXXRecordDecl {{.*}} <col:1, col:7> col:7 in M hidden implicit class Constexpr{{( ReachableWhenImported)?}}
+// CHECK-MOD-NEXT: | `-CXXMethodDecl {{.*}} <line:15:3, col:22> col:18 in M hidden constexpr z 'void ()'{{( ReachableWhenImported)?}} implicit-inline
+
+// CHECK-MOD: `-CXXRecordDecl {{.*}} <line:18:1, line:20:1> line:18:7 in M hidden class Consteval{{( ReachableWhenImported)?}} definition
+// CHECK-MOD: |-CXXRecordDecl {{.*}} <col:1, col:7> col:7 in M hidden implicit class Consteval{{( ReachableWhenImported)?}}
+// CHECK-MOD-NEXT: `-CXXMethodDecl {{.*}} <line:19:3, col:22> col:18 in M hidden consteval z 'void ()'{{( ReachableWhenImported)?}} implicit-inline

shafik

The link you gave for the regression is not a llvm commit, is that the correct commit you meant to point to?

…fined in-class This correct issue, when the functions declared as `constexpr` are `consteval`, are not considered to be inline (`isInlined()` is false) when defined inside class attached to named module: ```c++ export module mod; struct Clazz { constexpr void f1() { } // non-inline constexpr void f2(); friend constexpr void f3() {} // non-inline }; constexpr void Clazz::f3() {} // inline ``` This conflicts with [decl.constexpr] p1: > A function or static data member declared with the constexpr or consteval specifier on its first declaration is implicitly an inline function or variable ([dcl.inline]). If any declaration of a function or function template has a constexpr or consteval specifier, then all its declarations shall contain the same specifier/) This regression was introduced by llvm@97af17c5, where the inline of such function was accidentally removed The corresponding wording in [class.friend] and p6 [class.mfct] p1 uses "if" and not "if and only if", thus does not imply that these are only cases where such functions are inline.

tomasz-kaminski-sonarsource · 2024-09-21T04:40:29Z

Thank you for pointing out the mistake; the correct link is 97af17c5.

cor3ntin

I think this looks reasonable but I'd like @iains or @ChuanqiXu9 to review too.
Can you add a changelog entry?

cor3ntin · 2024-09-21T07:46:27Z

clang/lib/Sema/SemaDecl.cpp

+    // the global module). This does not affect declarations, that are already
+    // inline, for example due being declared `inline` or `consteval`


Suggested change

// the global module). This does not affect declarations, that are already

// inline, for example due being declared `inline` or `consteval`

// the global module). This does not affect declarations that are already

// inline (whether explicitly or implicitly by being declared constexpr, etc)

Updated the comment and added entry in bug fixes changes.

tomasz-kaminski-sonarsource · 2024-09-23T10:57:02Z

I tested this on 19.0.1 and missed the fact that it was fixed on main by 74ac96a.
I will rework this in NFC comment, that would add tests.

tomasz-kaminski-sonarsource · 2024-09-23T12:48:49Z

I have pushed the changes and updated the title and description to reflect no function aspect of this PR.

ChuanqiXu9

LGTM.

clang/test/CXX/class/class.mfct/p1-cxx20.cpp

llvmbot added clang Clang issues not falling into any other category clang:frontend Language frontend issues, e.g. anything involving "Sema" labels Sep 20, 2024

shafik requested a review from ChuanqiXu9 September 20, 2024 23:10

shafik reviewed Sep 20, 2024

View reviewed changes

tomasz-kaminski-sonarsource force-pushed the tk/modules-inline-fix branch from ed0bc29 to 6ff9964 Compare September 21, 2024 04:38

cor3ntin requested a review from iains September 21, 2024 07:43

cor3ntin reviewed Sep 21, 2024

View reviewed changes

Updated comment and docs

215b045

Reworked for NFC

6e641fe

tomasz-kaminski-sonarsource changed the title ~~[C++20][Modules] Restore inliness of constexpr/consteval functions defined in-class~~ [C++20][Modules] NFC Reworked handling of inline for functions defined in-class Sep 23, 2024

tomasz-kaminski-sonarsource changed the title ~~[C++20][Modules] NFC Reworked handling of inline for functions defined in-class~~ [C++20][Modules] NFC Reworked handling of inline for functions defined in class Sep 23, 2024

ChuanqiXu9 approved these changes Sep 24, 2024

View reviewed changes

Merge branch 'main' into tk/modules-inline-fix

0f814b6

tomasz-kaminski-sonarsource commented Sep 24, 2024

View reviewed changes

clang/test/CXX/class/class.mfct/p1-cxx20.cpp Outdated Show resolved Hide resolved

Added missing endline.

cc36632

tomasz-kaminski-sonarsource merged commit 4ac1416 into llvm:main Sep 24, 2024
8 checks passed

tomasz-kaminski-sonarsource deleted the tk/modules-inline-fix branch September 24, 2024 07:37

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Uh oh!

[C++20][Modules] NFC Reworked handling of inline for functions defined in class #109470

[C++20][Modules] NFC Reworked handling of inline for functions defined in class #109470

Uh oh!

tomasz-kaminski-sonarsource commented Sep 20, 2024 •

edited

Loading

Uh oh!

llvmbot commented Sep 20, 2024

Uh oh!

shafik left a comment

Uh oh!

tomasz-kaminski-sonarsource commented Sep 21, 2024 •

edited

Loading

Uh oh!

cor3ntin left a comment

Uh oh!

cor3ntin Sep 21, 2024

Uh oh!

tomasz-kaminski-sonarsource Sep 21, 2024

Uh oh!

tomasz-kaminski-sonarsource commented Sep 23, 2024

Uh oh!

tomasz-kaminski-sonarsource commented Sep 23, 2024 •

edited

Loading

Uh oh!

ChuanqiXu9 left a comment

Uh oh!

Uh oh!

Uh oh!

Uh oh!

		// the global module). This does not affect declarations, that are already
		// inline, for example due being declared `inline` or `consteval`

[C++20][Modules] NFC Reworked handling of inline for functions defined in class #109470

[C++20][Modules] NFC Reworked handling of inline for functions defined in class #109470

Uh oh!

Conversation

tomasz-kaminski-sonarsource commented Sep 20, 2024 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

llvmbot commented Sep 20, 2024

Uh oh!

shafik left a comment

Choose a reason for hiding this comment

Uh oh!

tomasz-kaminski-sonarsource commented Sep 21, 2024 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

cor3ntin left a comment

Choose a reason for hiding this comment

Uh oh!

cor3ntin Sep 21, 2024

Choose a reason for hiding this comment

Uh oh!

tomasz-kaminski-sonarsource Sep 21, 2024

Choose a reason for hiding this comment

Uh oh!

tomasz-kaminski-sonarsource commented Sep 23, 2024

Uh oh!

tomasz-kaminski-sonarsource commented Sep 23, 2024 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

ChuanqiXu9 left a comment

Choose a reason for hiding this comment

Uh oh!

Uh oh!

Uh oh!

Uh oh!

tomasz-kaminski-sonarsource commented Sep 20, 2024 •

edited

Loading

tomasz-kaminski-sonarsource commented Sep 21, 2024 •

edited

Loading

tomasz-kaminski-sonarsource commented Sep 23, 2024 •

edited

Loading