[PATCH] D78100: [AST] Suppress the spammy "attempt to use a deleted fucntion" diagnostic.

[Richard Smith - zygoloid via Phabricator via cfe-commits]

rsmith added a comment.

In D78100#1981620 <https://reviews.llvm.org/D78100#1981620>, @sammccall wrote:

> Sorry to go back and forth on this, but I'm not sure whether these diagnostics are actually spam to be suppressed. I think @adamcz mentioned these today as reasonable diagnostics we're enabling.
>
> @rsmith do you have an opinion here?


My initial reaction was certainly that it wasn't obvious why an initialization error would necessarily have anything to do with a destruction error. But this line of thinking helped me: suppose we would encounter both an (unrecoverable) initialization error *and* an error during destruction. How likely is it that they have a common cause? My guess would be way more than half the time. Given that we know we're on an error recovery path, and that we've already produced an unrecoverable error during initialization, skipping the destruction checks is probably the better choice. Even if we're wrong, the worst case is that the programmer fixes the initialization error and is then presented with a destruction error that they didn't see before, which doesn't seem all that bad of an outcome.

(Generally I don't think we need to be sure that errors would be correlated to suppress latter errors as being follow-on diagnostics from earlier errors, and should lean towards suppressing diagnostics in order to make the second and subsequent diagnostic as likely as possible to be meaningful and useful.)



================
Comment at: clang/lib/Sema/SemaDecl.cpp:11985-11986
           });
       if (Res.isInvalid()) {
         VDecl->setInvalidDecl();
       } else if (Res.get() != Args[Idx]) {
----------------
Should we attach a `RecoveryExpr` initializer in this case?


================
Comment at: clang/lib/Sema/SemaDecl.cpp:11998-12001
     if (Result.isInvalid()) {
       VDecl->setInvalidDecl();
       return;
     }
----------------
Should we attach a `RecoveryExpr` initializer in this case?


================
Comment at: clang/lib/Sema/SemaDecl.cpp:12268
 /// of sanity.
 void Sema::ActOnInitializerError(Decl *D) {
   // Our main concern here is re-establishing invariants like "a
----------------
Should we attach a `RecoveryExpr` initializer in this case too?


================
Comment at: clang/lib/Sema/SemaDecl.cpp:12562-12563
+    } else if (Init.isInvalid()) {
+      // If default-init fails, leave var uninitialized but valid, and build
+      // a recovery-expr for recovery.
+      auto RecoveryExpr =
----------------
I don't think this is accurate (we didn't "leave var uninitialized"). Maybe something like: "If default-init fails, attach an initializer that's marked invalid to track that initialization was attempted and failed."


================
Comment at: clang/test/CXX/class.access/p4.cpp:1
-// RUN: %clang_cc1 -triple %itanium_abi_triple -fcxx-exceptions -fexceptions -fsyntax-only -verify -std=c++98 %s
-// RUN: %clang_cc1 -triple %itanium_abi_triple -fcxx-exceptions -fexceptions -fsyntax-only -verify -std=c++11 %s
-// RUN: %clang_cc1 -triple %itanium_abi_triple -fcxx-exceptions -fexceptions -fsyntax-only -verify %s
-// RUN: %clang_cc1 -triple x86_64-windows-msvc -fms-compatibility-version=19 -fcxx-exceptions -fexceptions -fsyntax-only -verify -std=c++98 %s
-// RUN: %clang_cc1 -triple x86_64-windows-msvc -fms-compatibility-version=19 -fcxx-exceptions -fexceptions -fsyntax-only -verify -std=c++11 %s
-// RUN: %clang_cc1 -triple x86_64-windows-msvc -fms-compatibility-version=19 -fcxx-exceptions -fexceptions -fsyntax-only -verify %s
+// RUN: %clang_cc1 -frecovery-ast -triple %itanium_abi_triple -fcxx-exceptions -fexceptions -fsyntax-only -verify -std=c++98 %s
+// RUN: %clang_cc1 -frecovery-ast -triple %itanium_abi_triple -fcxx-exceptions -fexceptions -fsyntax-only -verify -std=c++11 %s
----------------
I'm not really happy about improving our quality of diagnostics only under `-frecovery-ast`. Do we really need that flag? The way I see it, we can divide the users of Clang up into:

 * We want valid code (eg, as a compiler): if we hit an error, it doesn't matter too much whether we build `RecoveryExpr`s or not, since we're on a path to bailing out anyway. If `RecoveryExpr`s allow us to improve our diagnostics, we should build them. (Exception: if we want valid code or to get a simple pass/fail as early as possible, maybe not.)
 * We want to accept invalid code (eg, tooling): if we hit an error, we probably want to retain as much information as we reasonably can about the non-erroneous parts of the program.

So I think at least the default should be to build `RecoveryExpr`s, and maybe we can remove the flag entirely?


Repository:
  rG LLVM Github Monorepo

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

https://reviews.llvm.org/D78100