Quuxplusone added inline comments.
================
Comment at: clang/include/clang/Basic/DiagnosticGroups.td:158
+def DeprecatedCopy : DiagGroup<"deprecated-copy",
[DeprecatedCopyUserProvided]>;
+def DeprecatedCopyDtor : DiagGroup<"deprecated-copy-dtor",
[DeprecatedCopyDtorUserProvided]>;
def DeprecatedDeclarations : DiagGroup<"deprecated-declarations">;
----------------
xbolva00 wrote:
> Quuxplusone wrote:
> > If we're going to provide these options at all, I think it would be more
> > grammatical to call them `-Wdeprecated-copy-with-deleted-copy` and
> > `-Wdeprecated-copy-with-deleted-dtor`.
> >
> > The existing code is already confusing by talking about a "copy dtor" as if
> > that's a thing; I think it makes it even worse to talk about "deprecated
> > copy user provided," since the actually deprecated thing is the
> > //implicitly generated// copy.
> >
> > I get that we're trying to be terse, and also somewhat hierarchical in
> > putting all the `-Wdeprecated-copy`-related warnings under
> > `-Wdeprecated-copy-foo-bar`; but the current names are too far into the
> > ungrammatical realm for me personally.
> Yeah, better names are welcome :)
Do the current names match existing practice in GCC or anything?
I continue to opine that these options are poorly named. My best suggestion is
`deprecated-copy`, `deprecated-copy-with-dtor`,
`deprecated-copy-with-deleted-copy`, `deprecated-copy-with-deleted-dtor` —
operating on the (wrong?) assumption that absolutely the only difference
between "user-declared" and "user-provided" corresponds to "user-declared as
deleted."
Even if everything else remains the same, the internal identifier
`warn_deprecated_copy_dtor_operation_user_provided` should certainly be
`warn_deprecated_copy_operation_dtor_user_provided` — the phrase that goes
together is "copy operation", not "dtor operation".
Your "deprecated-dtor-user-provided.cpp" passes
`-Wdeprecated-copy-dtor-user-provided`, but the message text talks about
"user-//declared//." Shouldn't the spelling of the option reflect the wording
of the message text? This isn't important from the POV of someone who's just
going to look at the message text and fix their code, but it's important from
the POV of someone who's going to use `-Wno-` and wants to know exactly which
bad situations they're ignoring. (Also, the name of the file should match the
spelling of the option.)
================
Comment at: clang/lib/Sema/SemaDeclCXX.cpp:13948
+ S.Diag(UserDeclaredOperation->getLocation(), DiagID)
+ << RD << /*copy assignment*/ IsCopyAssignment;
}
----------------
The `/* copy assignment */` comment is probably no longer useful.
================
Comment at: clang/test/SemaCXX/deprecated-copy.cpp:7
+#ifdef NO_USER_PROVIDED
+// expected-no-diagnostics
+#endif
----------------
Quuxplusone wrote:
> xbolva00 wrote:
> > xbolva00 wrote:
> > > Quuxplusone wrote:
> > > > I'm fairly confident this should just be two different test files.
> > > > Also, if a test file has an un-ifdeffed `// expected-no-diagnostics`
> > > > plus an un-ifdeffed `// expected-note ...`, which one wins?
> > > ifdef is here
> > >
> > > and ifNdef is below :)
> > and DEPRECATED_COPY_DTOR is in own "code block"
> Ah, you're right, I had missed that line 18 was still controlled by the
> `#ifdef DEPRECATED_COPY_DTOR` condition.
> I still think this should be three(!) different files. Line 2 doesn't even
> look right to me: shouldn't it be `-Wdeprecated-copy
> -Wno-deprecated-copy-user-provided`?
>
> I just did a `git grep 'RUN:' | grep '[-]Wno-' | grep -v '[-]W[^n]'` and
> found a massive number of tests that put `-Wno-foo` on the command line
> without putting `-Wbar` anywhere on the same line; I suspect many of these
> are bugs.
I've lost track of what I was trying to say here and whether I was right ;) but
I assume we can consider this comment thread "resolved"?
================
Comment at: clang/test/SemaCXX/deprecated-dtor-user-provided.cpp:6
+ int *ptr;
+ ~A(); // expected-warning {{definition of implicit copy constructor for 'A'
is deprecated because it has a user-declared destructor}}
+};
----------------
Do you have a test case for when an operation is defaulted as deleted?
I actually don't understand Clang's/GCC's current behavior in this area.
https://godbolt.org/z/9h3qqP
Of these three situations, do //any// of them rely on deprecated behavior?
(And vice versa, what if we delete the copy constructor and try to use the
implicit copy-assignment operator?)
================
Comment at: clang/test/SemaCXX/deprecated.cpp:108
+ }; // expected-warning@-1 {{definition of
implicit copy assignment operator for 'DefaultedDtor' is deprecated because it
has a user-declared destructor}}
+ DefaultedDtor d1, d2(d1); // expected-note {{in implicit copy
constructor for 'DeprecatedCopy::DefaultedDtor' first required here}}
+ void h() { d1 = d2; } // expected-note {{in implicit copy assignment
operator for 'DeprecatedCopy::DefaultedDtor' first required here}}
----------------
Nit: I would prefer to see the declarations of d1 and d2 on individual lines,
both for style and to prove that d1's declaration triggers no warnings.
CHANGES SINCE LAST ACTION
https://reviews.llvm.org/D79714/new/
https://reviews.llvm.org/D79714
_______________________________________________
cfe-commits mailing list
cfe-commits@lists.llvm.org
https://lists.llvm.org/cgi-bin/mailman/listinfo/cfe-commits
