[clang] [Clang][Sema]: Allow copy constructor side effects (PR #81127)

https://github.com/AaronBallman closed https://github.com/llvm/llvm-project/pull/81127 ___ cfe-commits mailing list cfe-commits@lists.llvm.org https://lists.llvm.org/cgi-bin/mailman/listinfo/cfe-commits

[clang] [Clang][Sema]: Allow copy constructor side effects (PR #81127)

vinayakdsci wrote: > 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 ha

[clang] [Clang][Sema]: Allow copy constructor side effects (PR #81127)

https://github.com/vinayakdsci updated https://github.com/llvm/llvm-project/pull/81127 >From d4df9a7bef0f993d0c092af5f5af61202dc3effe Mon Sep 17 00:00:00 2001 From: Vinayak Dev Date: Thu, 8 Feb 2024 17:29:44 +0530 Subject: [PATCH] [Clang][Sema]: Allow copy constructor side effects --- clang/d

[clang] [Clang][Sema]: Allow copy constructor side effects (PR #81127)

https://github.com/AaronBallman approved this pull request. 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? https://github.com/llvm/llvm-project/pull/81127 ___

[clang] [Clang][Sema]: Allow copy constructor side effects (PR #81127)

AaronBallman wrote: > @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 el

[clang] [Clang][Sema]: Allow copy constructor side effects (PR #81127)

vinayakdsci wrote: @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

[clang] [Clang][Sema]: Allow copy constructor side effects (PR #81127)

https://github.com/AaronBallman edited https://github.com/llvm/llvm-project/pull/81127 ___ cfe-commits mailing list cfe-commits@lists.llvm.org https://lists.llvm.org/cgi-bin/mailman/listinfo/cfe-commits

[clang] [Clang][Sema]: Allow copy constructor side effects (PR #81127)

https://github.com/AaronBallman commented: 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). https://github.com/llvm/llvm-project/pull/81127

[clang] [Clang][Sema]: Allow copy constructor side effects (PR #81127)

@@ -173,6 +173,10 @@ Bug Fixes in This Version for logical operators in C23. Fixes (`#64356 `_). +- Clang now doesn't produce false-positive warning `-Wunused-variable` + for variables created through copy initialization

[clang] [Clang][Sema]: Allow copy constructor side effects (PR #81127)

vinayakdsci wrote: @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! https://github.com/llvm/llvm-project/pull/81127 ___ cfe-c

[clang] [Clang][Sema]: Allow copy constructor side effects (PR #81127)

vinayakdsci wrote: @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

[clang] [Clang][Sema]: Allow copy constructor side effects (PR #81127)

vinayakdsci wrote: 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

[clang] [Clang][Sema]: Allow copy constructor side effects (PR #81127)

cor3ntin wrote: @AaronBallman does not handling c++11 make sense to you? Thanks! https://github.com/llvm/llvm-project/pull/81127 ___ cfe-commits mailing list cfe-commits@lists.llvm.org https://lists.llvm.org/cgi-bin/mailman/listinfo/cfe-commits

[clang] [Clang][Sema]: Allow copy constructor side effects (PR #81127)

vinayakdsci wrote: @cor3ntin gentle ping. Could you direct on what other changes are required? Thanks! https://github.com/llvm/llvm-project/pull/81127 ___ cfe-commits mailing list cfe-commits@lists.llvm.org https://lists.llvm.org/cgi-bin/mailman/listi

[clang] [Clang][Sema]: Allow copy constructor side effects (PR #81127)

vinayakdsci wrote: 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. Sorr

[clang] [Clang][Sema]: Allow copy constructor side effects (PR #81127)

cor3ntin wrote: 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 https://github.com/llvm/llvm-project/issues/79518 https://github.com/llvm/llvm-project/pull/81

[clang] [Clang][Sema]: Allow copy constructor side effects (PR #81127)

vinayakdsci wrote: @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! https://github.com/llvm/llvm-project/pull/81127 ___ cfe-commits mailing list cfe-commits@lis

[clang] [Clang][Sema]: Allow copy constructor side effects (PR #81127)

vinayakdsci wrote: @cor3ntin is this good to go? Thanks! https://github.com/llvm/llvm-project/pull/81127 ___ cfe-commits mailing list cfe-commits@lists.llvm.org https://lists.llvm.org/cgi-bin/mailman/listinfo/cfe-commits

[clang] [Clang][Sema]: Allow copy constructor side effects (PR #81127)

vinayakdsci wrote: > LGTM Make sure @cor3ntin is happy, too. Thanks a lot! I have requested a review from @cor3ntin too. Thanks for your guidance! https://github.com/llvm/llvm-project/pull/81127 ___ cfe-commits mailing list cfe-commits@lists.llvm.org

[clang] [Clang][Sema]: Allow copy constructor side effects (PR #81127)

https://github.com/Endilll approved this pull request. LGTM Make sure @cor3ntin is happy, too. https://github.com/llvm/llvm-project/pull/81127 ___ cfe-commits mailing list cfe-commits@lists.llvm.org https://lists.llvm.org/cgi-bin/mailman/listinfo/cfe-c

[clang] [Clang][Sema]: Allow copy constructor side effects (PR #81127)

vinayakdsci wrote: I hope this does it! https://github.com/llvm/llvm-project/pull/81127 ___ cfe-commits mailing list cfe-commits@lists.llvm.org https://lists.llvm.org/cgi-bin/mailman/listinfo/cfe-commits

[clang] [Clang][Sema]: Allow copy constructor side effects (PR #81127)

https://github.com/vinayakdsci updated https://github.com/llvm/llvm-project/pull/81127 >From 335d2277881af6737ba899417121c321ce4d098f Mon Sep 17 00:00:00 2001 From: Vinayak Dev Date: Thu, 8 Feb 2024 17:29:44 +0530 Subject: [PATCH] [Clang][Sema]: Allow copy constructor side effects --- clang/d

[clang] [Clang][Sema]: Allow copy constructor side effects (PR #81127)

@@ -183,7 +185,12 @@ void foo(int size) { NonTriviallyDestructible array[2]; // no warning NonTriviallyDestructible nestedArray[2][2]; // no warning + // Copy initialzation gives warning before C++17 +#if __cplusplus <= 201402L Foo fooScalar = 1; // expected-warning {

[clang] [Clang][Sema]: Allow copy constructor side effects (PR #81127)

@@ -183,7 +185,12 @@ void foo(int size) { NonTriviallyDestructible array[2]; // no warning NonTriviallyDestructible nestedArray[2][2]; // no warning + // Copy initialzation gives warning before C++17 +#if __cplusplus <= 201402L Foo fooScalar = 1; // expected-warning {

[clang] [Clang][Sema]: Allow copy constructor side effects (PR #81127)

vinayakdsci wrote: @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! https://github.com/llvm/llvm-project/pull/81127 __

[clang] [Clang][Sema]: Allow copy constructor side effects (PR #81127)

https://github.com/vinayakdsci updated https://github.com/llvm/llvm-project/pull/81127 >From f2f9ba7a5399f259f4970f78803a02dc5689daff Mon Sep 17 00:00:00 2001 From: Vinayak Dev Date: Thu, 8 Feb 2024 17:29:44 +0530 Subject: [PATCH] [Clang][Sema]: Allow copy constructor side effects --- clang/d

[clang] [Clang][Sema]: Allow copy constructor side effects (PR #81127)

@@ -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-variabl

[clang] [Clang][Sema]: Allow copy constructor side effects (PR #81127)

@@ -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-variabl

[clang] [Clang][Sema]: Allow copy constructor side effects (PR #81127)

@@ -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-variabl

[clang] [Clang][Sema]: Allow copy constructor side effects (PR #81127)

@@ -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-variabl

[clang] [Clang][Sema]: Allow copy constructor side effects (PR #81127)

@@ -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-variabl

[clang] [Clang][Sema]: Allow copy constructor side effects (PR #81127)

@@ -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-variabl

[clang] [Clang][Sema]: Allow copy constructor side effects (PR #81127)

@@ -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-variabl

[clang] [Clang][Sema]: Allow copy constructor side effects (PR #81127)

https://github.com/vinayakdsci edited https://github.com/llvm/llvm-project/pull/81127 ___ cfe-commits mailing list cfe-commits@lists.llvm.org https://lists.llvm.org/cgi-bin/mailman/listinfo/cfe-commits

[clang] [Clang][Sema]: Allow copy constructor side effects (PR #81127)

@@ -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-variabl

[clang] [Clang][Sema]: Allow copy constructor side effects (PR #81127)

@@ -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-variabl

[clang] [Clang][Sema]: Allow copy constructor side effects (PR #81127)

@@ -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-variabl

[clang] [Clang][Sema]: Allow copy constructor side effects (PR #81127)

vinayakdsci wrote: > 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! https://github.com/llvm/llvm-project/pull/81127

[clang] [Clang][Sema]: Allow copy constructor side effects (PR #81127)

https://github.com/vinayakdsci updated https://github.com/llvm/llvm-project/pull/81127 >From 2ecea54704910bc4cd47fa8dfb4f8af370dfb398 Mon Sep 17 00:00:00 2001 From: Vinayak Dev Date: Thu, 8 Feb 2024 17:29:44 +0530 Subject: [PATCH] [Clang][Sema]: Allow copy constructor side effects --- clang/d

[clang] [Clang][Sema]: Allow copy constructor side effects (PR #81127)

cor3ntin wrote: 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! https://github.com/llvm/llvm-project/pull/81127 ___

[clang] [Clang][Sema]: Allow copy constructor side effects (PR #81127)

https://github.com/vinayakdsci updated https://github.com/llvm/llvm-project/pull/81127 >From be1f3528d9fa90bf0fe930744f9df054d0b45425 Mon Sep 17 00:00:00 2001 From: Vinayak Dev Date: Thu, 8 Feb 2024 17:29:44 +0530 Subject: [PATCH] [Clang][Sema]: Allow copy constructor side effects --- clang/l

[clang] [Clang][Sema]: Allow copy constructor side effects (PR #81127)

github-actions[bot] wrote: :warning: C/C++ code formatter, clang-format found issues in your code. :warning: You can test this locally with the following command: ``bash git-clang-format --diff a643ab852a63a14dba86e031247734c5e3d5adb9 3d06ced172a76490c13f892d9165d8cf2702eb87 --

[clang] [Clang][Sema]: Allow copy constructor side effects (PR #81127)

llvmbot wrote: @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.

[clang] [Clang][Sema]: Allow copy constructor side effects (PR #81127)

https://github.com/vinayakdsci created https://github.com/llvm/llvm-project/pull/81127 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. N