Page MenuHomePhabricator

[Diagnostics] Put "deprecated copy" warnings into -Wdeprecated-copy
ClosedPublic

Authored by xbolva00 on Fri, Nov 15, 2:41 PM.

Details

Summary

GCC 9 added -Wdeprecated-copy (as part of -Wextra). This diagnostic is already implemented in Clang too, just hidden under -Wdeprecated (not on by default).
This patch adds -Wdeprecated-copy and makes it compatible with GCC 9+.
This diagnostic is heavily tested in deprecated.cpp, so I added simple tests just to check we warn when new flag/-Wextra is enabled.

Diff Detail

Event Timeline

xbolva00 created this revision.Fri, Nov 15, 2:41 PM
Herald added a project: Restricted Project. · View Herald TranscriptFri, Nov 15, 2:41 PM
Herald added a subscriber: cfe-commits. · View Herald Transcript

Generally OK, but I'm concerned about including this in -Wextra. In particular, the deprecation of copy/move operations when a destructor is explicitly declared has experimentally been found to have a high false-positive rate. Does GCC include this in -Wextra? (Its documentation doesn't seem to say so.)

xbolva00 updated this revision to Diff 229658.Fri, Nov 15, 3:53 PM

Added -Wdeprecated-copy-dtor (not part of -Wextra). Now we should be fully compatible with GCC.

xbolva00 updated this revision to Diff 229660.Fri, Nov 15, 3:54 PM

Newline fixed.

Generally OK, but I'm concerned about including this in -Wextra. In particular, the deprecation of copy/move operations when a destructor is explicitly declared has experimentally been found to have a high false-positive rate. Does GCC include this in -Wextra? (Its documentation doesn't seem to say so.)

Yes, right. Fixed. New revision should be OK.

Harbormaster completed remote builds in B41073: Diff 229660.
dblaikie accepted this revision.Fri, Nov 22, 11:56 AM

Looks good to me - can handle anything else from @rsmith in post-commit review.

This revision is now accepted and ready to land.Fri, Nov 22, 11:56 AM
This revision was automatically updated to reflect the committed changes.
hans added a subscriber: hans.Mon, Nov 25, 6:31 AM

We're now getting a ton of warnings about this in Chromium. (https://crbug.com/1028110)

And I'm not really sure what the warning wants us to do?

Add copy ctor/op= manually.

If you have a lot of warnings, you can use -Wno-deprecated-copy.

hans added a comment.Mon, Nov 25, 8:56 AM

Add copy ctor/op= manually.

Does the warning mean that the implicitly defined copy ctor is not going to work in some later version of the language?

I'm just trying to understand what it's warning about. If it's pointing out bugs in our code, we should fix them obviously.

Why do we emit this before C++20? It's only deprecated in C++20 onwards, no? Shouldn't this be default-enabled based off language version instead of being in -Wextra? No other language deprecation warning so far (register etc) has worked like this warning does.

(Also, this being in -Wextra has confused users for us: We added -Wno-deprecated-copy somewhere in our central build flags, and then some target added -Wextra later on and was confused that the -Wno-deprecated-copy flag didn't have an effect – but that's a minor point.)

Ah, Hans points out that the paper says that it's been deprecated in C++11, so this should either be on by default (and mention "C++11" somewhere), or it should be in -pedantic given that no other compiler diagnoses this and given that it fires heavily on existing code (including LLVM).

xbolva00 added a comment.EditedThu, Dec 5, 6:43 AM

No other compiler ? GCC 9 introduced it even in non pedantic mode. Warnings in LLVM are fixed.

Not sure if breaking compatibility with gcc is good idea. We wanted to unify behaviours here, indeed. Users should fix ther codebases - or they can use -Wno flag. Your suggestion to mention C++11 somewhere is reasonable, +1.

Re-re-reading http://www.open-std.org/jtc1/sc22/wg21/docs/papers/2018/p0619r4.html#3.2, the recommendation is "We will consider again for C++23". So I think this should be enabled with -std=c++20 at the earliest, since that's where the idea is from. But even there it's uncertain if it's going to happen. So I'd say put it in -pedantic, and possibly enable it by default with -std=c++20.