The issue reports that Clang(and GCC too) emits a warning when a variable is declared through copy initialization, which should not happen as copy initialization allows side effects.
Here is an MWE:
struct S {
S(int);
};
int main() {
S s(0); // no warning
S s2 = 0; // emits unused warning, but should not do so
S s3{0}; // no warning
}
However, the problem arises when we turn to the tests. It turns out that the changes work only for the C++17 and later standards, and the warning is still emitted when I run the fixed clang binary from my build with -std=c++14 or lower.
That creates a problem in the test file clang/test/SemaCXX/warn-unused-variables.cpp in which the tests are run against the current clang build and with -std=gnu++11 which is causing a failure because the fixed behavior is the opposite of what it expects( a warning where there shouldn’t be a warning).
A possible fix was to change -std=gnu++11 to -std=gnu++17 but reviewers don’t think it would be right to modify tested behavior modes.
So in conclusion, I just have some questions that I want to ask:
What exactly does the C++ standard say with regard to copy initialization with side effects, and are there any differences between C++14 and C++17 in this respect?
Would it be wrong to modify the current tests, and should I create a new test file to check this behavior for C++ standards C++17 and later?
When certain criteria are met, an implementation is allowed to omit the copy/move construction of a class object, even if the constructor selected for the copy/move operation and/or the destructor for the object have side effects.
It’s fine to modify existing tests, as long as you know how they should behave, and you increase coverage (e.g. add new language modes). Decreasing coverage is also possible, but requires consideration.
Perfect! Thank you so much for replying.
One other thing: Does Clang internally handle converting constructors?
I am bound to think that this failure of emitting a warning could be coming from the constructors with copy initialization being converting?
I found a number of FIXMEs in the code that handles this behaviour.
Other than that, I will change the PR as you suggest.
I had a question I wanted to ask about this issue.
If I am not wrong, the optimization of copy constructors was strictly optional before C++17, after which it became mandatory?
Wouldn’t that mean that the compiler should throw a warning when compiling with < C++17?
I am otherwise having a hard time generalizing this behavior to older standards, because the constructors are not considered eligible for copy elision in those cases.
This makes it necessary to modify the logic in BuildCXXConstructExpr() where the Elidable property is set for the CXXConstructExpr that the constructor call corresponds to.
It only accepts temporary objects of types that can be cast to a CXXRecordDecl, which most types that are assigned by an ‘=’ are not.
There are also a bunch of assertions in the code path that the compiler follows while compiling the code, which break when I try to make changes, and I am unable to decide how I should proceed.
I would really appreciate some help on deciding what the acceptable behavior actually is? Is it a bug for the compiler to throw a warning for standards before C++17 when a copy constructor with side effects is used, and if it is, how should I go about it?
In case it isn’t, I would love to know how I can improve the PR further.