[PATCH] D155475: [Clang][Sema] Add -Wctad-selects-copy to diagnose copy deduction candidate

[Corentin Jabot via Phabricator via cfe-commits]

cor3ntin added subscribers: philnik, ldionne, cor3ntin.
cor3ntin added a comment.

I have a couple questions but overall I think this is a great warning to add.
Sorry it took us a while to get to your PR



================
Comment at: clang/include/clang/Basic/DiagnosticGroups.td:1384
+def CTADSelectsCopy      : DiagGroup<"ctad-selects-copy">;
+def CTADMaybeUnintended  : DiagGroup<"ctad-maybe-unintended", [
+    CTADMaybeUnsupported, CTADSelectsCopy
----------------
Maybe we should just call that `Wctad`?


================
Comment at: clang/include/clang/Basic/DiagnosticSemaKinds.td:2491-2494
+def warn_ctad_selects_copy : Warning<
+  "%select{move|copy}0-constructing not wrapping a %1, "
+  "use copy-%select{list-|}2initialization to suppress this warning">,
+  InGroup<CTADSelectsCopy>, DefaultIgnore;
----------------
This message is a bit difficult to understand.
Maybe `The type of this expression is `Y<Z>` because it was deduced by CTAD from the copy constructor of Y?`
It's not an easy warning to phrase!


================
Comment at: clang/lib/Sema/SemaInit.cpp:22
 #include "clang/Basic/TargetInfo.h"
+#include "clang/Lex/Preprocessor.h"
 #include "clang/Sema/Designator.h"
----------------
I don't think that adding this header is necessary


================
Comment at: clang/lib/Sema/SemaInit.cpp:10932
+    DiagnoseCTADCopy();
+  } else if (!PP.getSourceManager().isInSystemHeader(Template->getLocation())) {
+    DiagnoseCTADCopy();
----------------
So the idea here is that the standard library would have protected against that by providing deduction guides?
Which is funny because the original issue was reported because they did not.
Do you have an example of when we should not emit the diagnostic for a library type?

@ldionne @philnik Opinions on this warning?


================
Comment at: clang/test/SemaTemplate/ctad.cpp:51
+#pragma clang diagnostic warning "-Wctad-selects-copy"
+// like std::revrse_iterator, the motivating example
+template <typename T> struct CTAD {
----------------



CHANGES SINCE LAST ACTION
  https://reviews.llvm.org/D155475/new/

https://reviews.llvm.org/D155475