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

[Haojian Wu via Phabricator via cfe-commits]

hokein marked an inline comment as done.
hokein added inline comments.


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


================
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
----------------
sammccall wrote:
> rsmith wrote:
> > Should we attach a `RecoveryExpr` initializer in this case too?
> Now we're really slipping into a different set of use-cases for RecoveryExpr... 
> I assume we're talking about a RecoveryExpr that has no children, at least in the short term, as parsing failures don't return the likely subexpressions found. So it's a pure error marker in the form of an AST node. Quite a lot of ExprError()s could become these...
> 
> Actually there's another virtue here: even without subexpressions, the RecoveryExpr can capture the token range. So VarDecl::getSourceRange() will include the malformed initializer, so tools can at least attribute these tokens to the right part of the AST.
yeah, I'm not sure how much benefit we can get from the recovery-expr in this case, if the initializer is failed to parse, we don't have any ast nodes.


================
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
----------------
sammccall wrote:
> rsmith wrote:
> > 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?
> Agree that want to flip the default to true, and maybe eventually get rid of it. But:
>  - we're still fighting quite a lot of new crash-on-invalids, and can't fix them all in one big patch. We plan to find more by rolling this out to a subset of google-internal IDE users (where we have good monitoring), the flag is needed for this.
>  - we expect this to be stable for C++ long before it can be enabled for C, because that requires making the C codepaths safe handle (at least error-)dependence. So there'll be a similar testing/improvement period later.
> 
> However I don't like adding -frecovery-ast to big existing tests, we lose coverage of today's production behavior. I think we may need to create new tests to show the effect of these changes, and clean them up after flipping the default.
yeah, it is not perfect, given that we are at intermediate stage. We plan to enable the flag for C++ by default (we did it once, but failed with many crashes), this means we'd eventually focus on '-frecovery-ast' only (at least C++), it seems not too harmful to add the -frecovery-ast flag to exiting tests at the moment. But it is not great.. 

Another way is to adjust existing tests to support both, but this seems non-trivial, maybe creating new tests for '-frecovery-ast' is a best way to go.

life can be easier if the flag is turned on by default. Currently, I have to maintain a local patch with a long tail of adjusted tests when doing recovery-expr developments, I need to run all tests with&without -frecovery-ast to make sure it doesn't introduce big issures.


Repository:
  rG LLVM Github Monorepo

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

https://reviews.llvm.org/D78100