[Clang][Sema]: Allow copy constructor side effects #81127

vinayakdsci · 2024-02-08T12:06:13Z

Copy constructors can have initialization with side effects, and thus clang should not emit a warning when -Wunused-variable is used in this context. Currently however, a warning is emitted.

Now, compilation happens without warnings.

llvmbot · 2024-02-08T12:06:44Z

@llvm/pr-subscribers-clang

Author: Vinayak Dev (vinayakdsci)

Changes

Fixes #79518

Copy constructors can have initialization with side effects, and thus clang should not emit a warning when -Wunused-variable is used in this context. Currently however, a warning is emitted.

Now, compilation happens without warnings.

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

2 Files Affected:

(modified) clang/lib/Sema/SemaDecl.cpp (+1-1)
(modified) clang/test/SemaCXX/warn-unused-variables.cpp (+26-2)

diff --git a/clang/lib/Sema/SemaDecl.cpp b/clang/lib/Sema/SemaDecl.cpp
index 18a5d93ab8e8c..7f78aef33f1e3 100644
--- a/clang/lib/Sema/SemaDecl.cpp
+++ b/clang/lib/Sema/SemaDecl.cpp
@@ -2044,7 +2044,7 @@ static bool ShouldDiagnoseUnusedDecl(const LangOptions &LangOpts,
           return false;
 
         if (Init) {
-          const auto *Construct = dyn_cast<CXXConstructExpr>(Init);
+          const auto *Construct = dyn_cast<CXXConstructExpr>(Init->IgnoreImpCasts());
           if (Construct && !Construct->isElidable()) {
             const CXXConstructorDecl *CD = Construct->getConstructor();
             if (!CD->isTrivial() && !RD->hasAttr<WarnUnusedAttr>() &&
diff --git a/clang/test/SemaCXX/warn-unused-variables.cpp b/clang/test/SemaCXX/warn-unused-variables.cpp
index b649c7d808935..92032a9c2b812 100644
--- a/clang/test/SemaCXX/warn-unused-variables.cpp
+++ b/clang/test/SemaCXX/warn-unused-variables.cpp
@@ -1,5 +1,5 @@
 // RUN: %clang_cc1 -fsyntax-only -Wunused-variable -Wunused-label -Wno-c++1y-extensions -verify %s
-// RUN: %clang_cc1 -fsyntax-only -Wunused-variable -Wunused-label -Wno-c++1y-extensions -verify -std=gnu++11 %s
+// RUN: %clang_cc1 -fsyntax-only -Wunused-variable -Wunused-label -Wno-c++1y-extensions -verify -std=gnu++17 %s
 template<typename T> void f() {
   T t;
   t = 17;
@@ -183,7 +183,7 @@ void foo(int size) {
   NonTriviallyDestructible array[2];  // no warning
   NonTriviallyDestructible nestedArray[2][2]; // no warning
 
-  Foo fooScalar = 1; // expected-warning {{unused variable 'fooScalar'}}
+  Foo fooScalar = 1; // no warning
   Foo fooArray[] = {1,2}; // expected-warning {{unused variable 'fooArray'}}
   Foo fooNested[2][2] = { {1,2}, {3,4} }; // expected-warning {{unused variable 'fooNested'}}
 }
@@ -297,3 +297,27 @@ void RAIIWrapperTest() {
 }
 
 } // namespace gh54489
+
+// Ensure that -Wunused-variable does not emit warning
+// on copy constructors with side effects
+namespace gh79518 {
+
+struct S {
+    S(int);
+};
+
+// With an initializer list
+struct A {
+  int x;
+  A(int x) : x(x) {}
+};
+
+void foo() {
+    S s(0); // no warning
+    S s2 = 0; // no warning
+    S s3{0}; // no warning
+
+    A a = 1; // no warning
+}
+
+} // namespace gh79518

github-actions · 2024-02-08T12:08:43Z

✅ With the latest revision this PR passed the C/C++ code formatter.

cor3ntin · 2024-02-08T15:28:10Z

This change needs a release note.
Please add an entry to clang/docs/ReleaseNotes.rst in the section the most adapted to the change, and referencing any Github issue this change fixes. Thanks!

vinayakdsci · 2024-02-08T16:03:22Z

This change needs a release note. Please add an entry to clang/docs/ReleaseNotes.rst in the section the most adapted to the change, and referencing any Github issue this change fixes. Thanks!

Done!

clang/test/SemaCXX/warn-unused-variables.cpp

vinayakdsci · 2024-02-09T15:59:58Z

@Endilll as you mention on the discourse, I have divided the tests as per the difference in the standards. The tests seem to be working fine. I hope you can review once you find time.

Thanks!

Endilll · 2024-02-10T14:36:45Z

clang/test/SemaCXX/warn-unused-variables.cpp

  Foo fooScalar = 1; // expected-warning {{unused variable 'fooScalar'}}
+#else
+  Foo fooScalar = 1; // no warning


Code around here can be replaced with a single line:

Foo fooScalar = 1; // cxx98-14-warning {{unused variable 'fooScalar'}}

Look at DR tests again how to make this happen (via -verify=):

llvm-project/clang/test/CXX/drs/dr0xx.cpp

Lines 1 to 3 in 3902f9b

// RUN: %clang_cc1 -std=c++98 %s -verify=expected,cxx98,cxx98-14 -fexceptions -fcxx-exceptions -pedantic-errors -Wno-bind-to-temporary-copy

// RUN: %clang_cc1 -std=c++11 %s -verify=expected,since-cxx11,cxx98-14,cxx11-14 -fexceptions -fcxx-exceptions -pedantic-errors -triple %itanium_abi_triple

// RUN: %clang_cc1 -std=c++14 %s -verify=expected,since-cxx11,cxx98-14,cxx11-14 -fexceptions -fcxx-exceptions -pedantic-errors -triple %itanium_abi_triple

Here is documentation for -verify if you feel you need it: https://clang.llvm.org/docs/InternalsManual.html#verifying-diagnostics

vinayakdsci · 2024-02-10T15:32:46Z

I hope this does it!

Endilll

LGTM
Make sure @cor3ntin is happy, too.

vinayakdsci · 2024-02-10T16:31:52Z

LGTM Make sure @cor3ntin is happy, too.

Thanks a lot! I have requested a review from @cor3ntin too. Thanks for your guidance!

vinayakdsci · 2024-02-11T13:45:20Z

@cor3ntin is this good to go?

Thanks!

vinayakdsci · 2024-02-12T11:34:00Z

@Endilll sorry for repeated pings, but I think @cor3ntin might be possibly busy.

If this looks good, can we go ahead with this PR?

Thanks!

cor3ntin · 2024-02-12T11:43:49Z

We have a policy against more than one ping per week, thanks.

FYI, if we are not going to fix the C++11 case, we should be very explicit about it. IE this PR is not a complete fix of #79518

vinayakdsci · 2024-02-12T12:28:16Z

Sorry about the pings, I think I got a little ahead of myself there.
But for C++11, we got to the conclusion that copy elision gives different behaviour for C++17 and later and thus different diagnostics?
In case I made a mistake, I'd be very happy to fix it.

Thanks
P.S. Sorry about the pings, please forgive me.

vinayakdsci · 2024-02-18T11:47:11Z

@cor3ntin gentle ping. Could you direct on what other changes are required?

Thanks!

cor3ntin · 2024-02-18T11:56:08Z

@AaronBallman does not handling c++11 make sense to you? Thanks!

vinayakdsci · 2024-02-18T12:32:58Z

OK, it looks like there should not be a warning in C++11 and 14 too, as copy initialization with side effects appears to be permitted sine C++11.
Could you direct me to the functions that Clang uses internally for implicit type conversion?
I am sure that using that I will be able to handle the other cases too.

Thanks!

vinayakdsci · 2024-02-25T15:57:59Z

@cor3ntin gentle ping. How exactly should I proceed? copy elision was made mandatory in C++17, so wouldn't a warning for C++ < 17 be allowable?

I am very happy to implement any suggestions you have for this PR, but I am unable to get the code to work for C++ < 17 because the constructors are not marked elidable in the code for these standards.

Specifically in the case where the class object is initialized by an '=' sign instead of an explicit argument being passed. As the RHS of the '=' is different from a CXXRecordDecl, isTemporaryObject() returns false and thus sets the constructor as non-elidable. This is where I am not sure on how to proceed, as changing this behavior breaks a number of assertions in the code path followed while compiling the source program. Either I could check if the RHS of the assignment is an rvalue, and then add an OR condition to set Elidable for the constructor to true. However, whenever the constructor is elidable, the codegen emits directly, and expects an aggregate type to passed in, which is not true for a scalar such as an int.

Also, this would involve a conversion process in the code?

I would really appreciate pointers on how to solve this problem, and how to improve this PR too.

Thanks!

vinayakdsci · 2024-02-28T18:25:21Z

@AaronBallman does this PR look good to go, or should I make any changes and updates? I would really be grateful for any pointers on how this can be improved.

Thanks a lot!

AaronBallman

I think we should handle pre-C++17 as well, because the constructors are called there: https://godbolt.org/z/YWjb8P67c (the ASTs are different but the runtime effects are not).

AaronBallman · 2024-02-29T14:53:18Z

clang/docs/ReleaseNotes.rst

+- Clang now doesn't produce false-positive warning `-Wunused-variable`
+  for variables created through copy initialization having side-effects in C++17 and later.
+  Fixes (`#79518 <https://github.com/llvm/llvm-project/issues/79518>`_).


Suggested change

- Clang now doesn't produce false-positive warning `-Wunused-variable`

for variables created through copy initialization having side-effects in C++17 and later.

Fixes (`#79518 <https://github.com/llvm/llvm-project/issues/79518>`_).

- Clang no longer produces a false-positive `-Wunused-variable` warning

for variables created through copy initialization having side-effects in C++17 and later.

Fixes (`#79518 <https://github.com/llvm/llvm-project/issues/79518>`_).

vinayakdsci · 2024-02-29T17:58:57Z

@AaronBallman C++17 and later mark all of these constructor calls as non-elidable after ignoring the implicit casts, while C++14 and before mark the constructor that is assigned by = as elidable, and hence the warning. Should the constructor also be marked as elidable in C++ 17 and later, or not marked as elidable in C++14 and earlier?

AaronBallman · 2024-02-29T19:41:21Z

@AaronBallman C++17 and later mark all of these constructor calls as non-elidable after ignoring the implicit casts, while C++14 and before mark the constructor that is assigned by = as elidable, and hence the warning. Should the constructor also be marked as elidable in C++ 17 and later, or not marked as elidable in C++14 and earlier?

I just looked into the code and now I think it is reasonable to leave the C++14 and earlier behavior alone on the assumption that it's good enough for now and because the C++17 and up changes are an improvement. Whether a constructor is or isn't elidable is complex and there's a fair number of FIXME comments around it, so I don't think we should tie that to this PR.

AaronBallman

Aside from some grammatical changes to the release note, the changes LGTM! Do you need someone to land this on your behalf once those changes are made?

vinayakdsci · 2024-02-29T20:28:23Z

Aside from some grammatical changes to the release note, the changes LGTM!

Thank you so much! I have made changes to the Release Notes as you suggested. Thanks for your review!

Do you need someone to land this on your behalf once those changes are made?

Yes, I don't have commit access at present, so I would really appreciate it if you or someone else could land it on my behalf.

Again, a big thanks!

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

vinayakdsci force-pushed the copy-initialization-side-effects branch from 3d06ced to be1f352 Compare February 8, 2024 12:11

cor3ntin requested a review from Endilll February 8, 2024 12:18

vinayakdsci force-pushed the copy-initialization-side-effects branch from be1f352 to 2ecea54 Compare February 8, 2024 16:02

Endilll reviewed Feb 8, 2024

View reviewed changes

clang/test/SemaCXX/warn-unused-variables.cpp Show resolved Hide resolved

vinayakdsci force-pushed the copy-initialization-side-effects branch from 2ecea54 to f2f9ba7 Compare February 9, 2024 15:56

vinayakdsci requested a review from Endilll February 10, 2024 14:23

Endilll reviewed Feb 10, 2024

View reviewed changes

vinayakdsci force-pushed the copy-initialization-side-effects branch from f2f9ba7 to 335d227 Compare February 10, 2024 15:32

Endilll approved these changes Feb 10, 2024

View reviewed changes

vinayakdsci requested a review from cor3ntin February 10, 2024 16:30

AaronBallman reviewed Feb 29, 2024

View reviewed changes

AaronBallman approved these changes Feb 29, 2024

View reviewed changes

[Clang][Sema]: Allow copy constructor side effects

d4df9a7

vinayakdsci force-pushed the copy-initialization-side-effects branch from 335d227 to d4df9a7 Compare February 29, 2024 20:21

AaronBallman merged commit 2b4d67b into llvm:main Mar 1, 2024

	// RUN: %clang_cc1 -std=c++98 %s -verify=expected,cxx98,cxx98-14 -fexceptions -fcxx-exceptions -pedantic-errors -Wno-bind-to-temporary-copy
	// RUN: %clang_cc1 -std=c++11 %s -verify=expected,since-cxx11,cxx98-14,cxx11-14 -fexceptions -fcxx-exceptions -pedantic-errors -triple %itanium_abi_triple
	// RUN: %clang_cc1 -std=c++14 %s -verify=expected,since-cxx11,cxx98-14,cxx11-14 -fexceptions -fcxx-exceptions -pedantic-errors -triple %itanium_abi_triple

[Clang][Sema]: Allow copy constructor side effects #81127

[Clang][Sema]: Allow copy constructor side effects #81127

Uh oh!

Conversation

vinayakdsci commented Feb 8, 2024

Uh oh!

llvmbot commented Feb 8, 2024

Uh oh!

github-actions bot commented Feb 8, 2024 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

cor3ntin commented Feb 8, 2024

Uh oh!

vinayakdsci commented Feb 8, 2024

Uh oh!

Uh oh!

vinayakdsci commented Feb 9, 2024

Uh oh!

Endilll Feb 10, 2024

Choose a reason for hiding this comment

Uh oh!

Endilll Feb 10, 2024

Choose a reason for hiding this comment

Uh oh!

vinayakdsci commented Feb 10, 2024

Uh oh!

Endilll left a comment

Choose a reason for hiding this comment

Uh oh!

vinayakdsci commented Feb 10, 2024

Uh oh!

vinayakdsci commented Feb 11, 2024

Uh oh!

vinayakdsci commented Feb 12, 2024

Uh oh!

cor3ntin commented Feb 12, 2024

Uh oh!

vinayakdsci commented Feb 12, 2024

Uh oh!

vinayakdsci commented Feb 18, 2024

Uh oh!

cor3ntin commented Feb 18, 2024

Uh oh!

vinayakdsci commented Feb 18, 2024

Uh oh!

vinayakdsci commented Feb 25, 2024

Uh oh!

vinayakdsci commented Feb 28, 2024

Uh oh!

AaronBallman left a comment

Choose a reason for hiding this comment

Uh oh!

AaronBallman Feb 29, 2024

Choose a reason for hiding this comment

Uh oh!

vinayakdsci commented Feb 29, 2024

Uh oh!

AaronBallman commented Feb 29, 2024

Uh oh!

AaronBallman left a comment

Choose a reason for hiding this comment

Uh oh!

vinayakdsci commented Feb 29, 2024

Uh oh!

Uh oh!

github-actions bot commented Feb 8, 2024 •

edited

Loading