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

[Haojian Wu via Phabricator via cfe-commits]

This revision was automatically updated to reflect the committed changes.
Closed by commit rGe90fb82f0f76: [AST] Suppress the spammy attempt to use 
a deleted fucntion diagnostic. (authored by hokein).

Changed prior to commit:
  https://reviews.llvm.org/D78100?vs=257689=258918#toc

Repository:
  rG LLVM Github Monorepo

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

https://reviews.llvm.org/D78100

Files:
  clang/lib/Sema/SemaDecl.cpp
  clang/lib/Sema/SemaDeclCXX.cpp
  clang/test/SemaCXX/recovery-default-init.cpp


Index: clang/test/SemaCXX/recovery-default-init.cpp
===
--- /dev/null
+++ clang/test/SemaCXX/recovery-default-init.cpp
@@ -0,0 +1,14 @@
+// RUN: %clang_cc1 %s -fsyntax-only -frecovery-ast -verify -std=c++11
+
+// NOTE: the test can be merged into existing tests once -frecovery-ast is on
+// by default.
+
+struct Foo { // expected-note {{candidate constructor (the implicit copy 
constructor) not viable}}
+  Foo(int); // expected-note {{candidate constructor not viable}}
+  ~Foo() = delete;
+};
+
+void test() {
+  // we expect the "attempt to use a deleted function" diagnostic is 
suppressed.
+  Foo foo; // expected-error {{no matching constructor for initialization of}}
+}
Index: clang/lib/Sema/SemaDeclCXX.cpp
===
--- clang/lib/Sema/SemaDeclCXX.cpp
+++ clang/lib/Sema/SemaDeclCXX.cpp
@@ -15002,6 +15002,10 @@
 
 void Sema::FinalizeVarWithDestructor(VarDecl *VD, const RecordType *Record) {
   if (VD->isInvalidDecl()) return;
+  // If initializing the variable failed, don't also diagnose problems with
+  // the desctructor, they're likely related.
+  if (VD->getInit() && VD->getInit()->containsErrors())
+return;
 
   CXXRecordDecl *ClassDecl = cast(Record->getDecl());
   if (ClassDecl->isInvalidDecl()) return;
Index: clang/lib/Sema/SemaDecl.cpp
===
--- clang/lib/Sema/SemaDecl.cpp
+++ clang/lib/Sema/SemaDecl.cpp
@@ -12554,12 +12554,17 @@
 InitializationSequence InitSeq(*this, Entity, Kind, None);
 ExprResult Init = InitSeq.Perform(*this, Entity, Kind, None);
 
-// If default-init fails, leave var uninitialized but valid, for recovery.
-
 if (Init.get()) {
   Var->setInit(MaybeCreateExprWithCleanups(Init.get()));
   // This is important for template substitution.
   Var->setInitStyle(VarDecl::CallInit);
+} else if (Init.isInvalid()) {
+  // If default-init fails, attach a recovery-expr initializer to track
+  // that initialization was attempted and failed.
+  auto RecoveryExpr =
+  CreateRecoveryExpr(Var->getLocation(), Var->getLocation(), {});
+  if (RecoveryExpr.get())
+Var->setInit(RecoveryExpr.get());
 }
 
 CheckCompleteVariableDeclaration(Var);


Index: clang/test/SemaCXX/recovery-default-init.cpp
===
--- /dev/null
+++ clang/test/SemaCXX/recovery-default-init.cpp
@@ -0,0 +1,14 @@
+// RUN: %clang_cc1 %s -fsyntax-only -frecovery-ast -verify -std=c++11
+
+// NOTE: the test can be merged into existing tests once -frecovery-ast is on
+// by default.
+
+struct Foo { // expected-note {{candidate constructor (the implicit copy constructor) not viable}}
+  Foo(int); // expected-note {{candidate constructor not viable}}
+  ~Foo() = delete;
+};
+
+void test() {
+  // we expect the "attempt to use a deleted function" diagnostic is suppressed.
+  Foo foo; // expected-error {{no matching constructor for initialization of}}
+}
Index: clang/lib/Sema/SemaDeclCXX.cpp
===
--- clang/lib/Sema/SemaDeclCXX.cpp
+++ clang/lib/Sema/SemaDeclCXX.cpp
@@ -15002,6 +15002,10 @@
 
 void Sema::FinalizeVarWithDestructor(VarDecl *VD, const RecordType *Record) {
   if (VD->isInvalidDecl()) return;
+  // If initializing the variable failed, don't also diagnose problems with
+  // the desctructor, they're likely related.
+  if (VD->getInit() && VD->getInit()->containsErrors())
+return;
 
   CXXRecordDecl *ClassDecl = cast(Record->getDecl());
   if (ClassDecl->isInvalidDecl()) return;
Index: clang/lib/Sema/SemaDecl.cpp
===
--- clang/lib/Sema/SemaDecl.cpp
+++ clang/lib/Sema/SemaDecl.cpp
@@ -12554,12 +12554,17 @@
 InitializationSequence InitSeq(*this, Entity, Kind, None);
 ExprResult Init = InitSeq.Perform(*this, Entity, Kind, None);
 
-// If default-init fails, leave var uninitialized but valid, for recovery.
-
 if (Init.get()) {
   Var->setInit(MaybeCreateExprWithCleanups(Init.get()));
   // This is important for template substitution.
   Var->setInitStyle(VarDecl::CallInit);
+} else if (Init.isInvalid()) {
+  // If default-init fails, attach a recovery-expr initializer to track
+  // that initialization was attempted 

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

[Sam McCall via Phabricator via cfe-commits]

sammccall added a comment.

OK LG without changes




Comment at: clang/lib/Sema/SemaDecl.cpp:12565
+  auto RecoveryExpr =
+  CreateRecoveryExpr(Var->getLocation(), Var->getEndLoc(), {});
+  if (RecoveryExpr.get())

hokein wrote:
> sammccall wrote:
> > This seems like it's going to claim some actual tokens, when the thing it 
> > represents doesn't cover any tokens.
> > 
> > I think both start/end source locations should be invalid.
> actually, I think it is still valuable to set the var location to 
> recovery-expr.
> ```
> Foo [[foo]]; // if there is a valid default ctor, we have a CtorExpr which 
> has the `foo` range; otherwise there is a recoveryExpr with the same range.
> ```
Yeah, invalid source range is scary too. Let's go with this and see if things 
break. I think more likely it'll be a mild annoyance like the range of 
CXXConstructExpr.

(In a perfect world maybe this would have a location but no range, or a 
SourceRange would have half-open semantics and could represent a point).


Repository:
  rG LLVM Github Monorepo

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

https://reviews.llvm.org/D78100



___
cfe-commits mailing list
cfe-commits@lists.llvm.org
https://lists.llvm.org/cgi-bin/mailman/listinfo/cfe-commits

[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:12565
+  auto RecoveryExpr =
+  CreateRecoveryExpr(Var->getLocation(), Var->getEndLoc(), {});
+  if (RecoveryExpr.get())

sammccall wrote:
> This seems like it's going to claim some actual tokens, when the thing it 
> represents doesn't cover any tokens.
> 
> I think both start/end source locations should be invalid.
actually, I think it is still valuable to set the var location to recovery-expr.
```
Foo [[foo]]; // if there is a valid default ctor, we have a CtorExpr which has 
the `foo` range; otherwise there is a recoveryExpr with the same range.
```


Repository:
  rG LLVM Github Monorepo

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

https://reviews.llvm.org/D78100



___
cfe-commits mailing list
cfe-commits@lists.llvm.org
https://lists.llvm.org/cgi-bin/mailman/listinfo/cfe-commits

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

[Sam McCall via Phabricator via cfe-commits]

sammccall accepted this revision.
sammccall added a comment.
This revision is now accepted and ready to land.

LG but please fix the sourcerange




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

rsmith wrote:
> hokein wrote:
> > 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.
> What I'm thinking is that we should have some kind of marker in the AST to 
> track that an initializer was provided for the variable but was malformed. 
> Right now, we use the "invalid" bit for that, which means that downstream 
> code that refers to the variable will have poor recovery. If we attach an 
> initialization expression marked with the "contains errors" bit instead, then 
> we can keep the `VarDecl` valid, and improve the recovery not necessarily for 
> this node but for things downstream of it.
> 
> (I guess ultimately it seems reasonable to me to use the same AST 
> representation for "no initializer was provided and implicit default init 
> failed", "an initializer was provided but we couldn't parse / semantically 
> analyze it", and "an initializer was provided but was incompatible with the 
> variable's type" -- except that in the third case we can track the expression 
> that we formed for the initializer.)
> 
> I don't think there's any urgency to this, and having both AST models for a 
> while (in different cases) doesn't seem like it's going to cause much pain.
Yeah, I think this makes sense. @hokein: even without child AST nodes, this 
will improve SelectionTree accuracy in clangd: go to definition on some part of 
an invalid initializer currently goes to the vardecl, and after adding 
RecoveryExpr here it'll go nowhere instead which is progress.

But the expr needs to have at least the source range, so this isn't a trivial 
change --> another patch?



Comment at: clang/lib/Sema/SemaDecl.cpp:12565
+  auto RecoveryExpr =
+  CreateRecoveryExpr(Var->getLocation(), Var->getEndLoc(), {});
+  if (RecoveryExpr.get())

This seems like it's going to claim some actual tokens, when the thing it 
represents doesn't cover any tokens.

I think both start/end source locations should be invalid.



Comment at: clang/lib/Sema/SemaDeclCXX.cpp:15007
+  // If initializing the variable failed, don't also diagnose problems with
+  // the desctructor, they're likely related.
+  if (VD->getInit() && VD->getInit()->containsErrors())

desctructor -> destructor
(sorry, you copied my typo)


Repository:
  rG LLVM Github Monorepo

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

https://reviews.llvm.org/D78100



___
cfe-commits mailing list
cfe-commits@lists.llvm.org
https://lists.llvm.org/cgi-bin/mailman/listinfo/cfe-commits

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

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

rsmith added inline comments.



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

hokein wrote:
> 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.
What I'm thinking is that we should have some kind of marker in the AST to 
track that an initializer was provided for the variable but was malformed. 
Right now, we use the "invalid" bit for that, which means that downstream code 
that refers to the variable will have poor recovery. If we attach an 
initialization expression marked with the "contains errors" bit instead, then 
we can keep the `VarDecl` valid, and improve the recovery not necessarily for 
this node but for things downstream of it.

(I guess ultimately it seems reasonable to me to use the same AST 
representation for "no initializer was provided and implicit default init 
failed", "an initializer was provided but we couldn't parse / semantically 
analyze it", and "an initializer was provided but was incompatible with the 
variable's type" -- except that in the third case we can track the expression 
that we formed for the initializer.)

I don't think there's any urgency to this, and having both AST models for a 
while (in different cases) doesn't seem like it's going to cause much pain.


Repository:
  rG LLVM Github Monorepo

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

https://reviews.llvm.org/D78100



___
cfe-commits mailing list
cfe-commits@lists.llvm.org
https://lists.llvm.org/cgi-bin/mailman/listinfo/cfe-commits

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

[Haojian Wu via Phabricator via cfe-commits]

hokein updated this revision to Diff 257689.
hokein marked 4 inline comments as done.
hokein added a comment.

address comments:

- don't modify the existing tests
- add new tests for -frecovery-ast only.


Repository:
  rG LLVM Github Monorepo

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

https://reviews.llvm.org/D78100

Files:
  clang/lib/Sema/SemaDecl.cpp
  clang/lib/Sema/SemaDeclCXX.cpp
  clang/test/SemaCXX/recovery-default-init.cpp


Index: clang/test/SemaCXX/recovery-default-init.cpp
===
--- /dev/null
+++ clang/test/SemaCXX/recovery-default-init.cpp
@@ -0,0 +1,14 @@
+// RUN: %clang_cc1 %s -fsyntax-only -frecovery-ast -verify -std=c++11
+
+// NOTE: the test can be merged into existing tests once -frecovery-ast is on
+// by default.
+
+struct Foo { // expected-note {{candidate constructor (the implicit copy 
constructor) not viable}}
+  Foo(int); // expected-note {{candidate constructor not viable}}
+  ~Foo() = delete;
+};
+
+void test() {
+  // we expect the "attempt to use a deleted function" diagnostic is 
suppressed.
+  Foo foo; // expected-error {{no matching constructor for initialization of}}
+}
Index: clang/lib/Sema/SemaDeclCXX.cpp
===
--- clang/lib/Sema/SemaDeclCXX.cpp
+++ clang/lib/Sema/SemaDeclCXX.cpp
@@ -15003,6 +15003,10 @@
 
 void Sema::FinalizeVarWithDestructor(VarDecl *VD, const RecordType *Record) {
   if (VD->isInvalidDecl()) return;
+  // If initializing the variable failed, don't also diagnose problems with
+  // the desctructor, they're likely related.
+  if (VD->getInit() && VD->getInit()->containsErrors())
+return;
 
   CXXRecordDecl *ClassDecl = cast(Record->getDecl());
   if (ClassDecl->isInvalidDecl()) return;
Index: clang/lib/Sema/SemaDecl.cpp
===
--- clang/lib/Sema/SemaDecl.cpp
+++ clang/lib/Sema/SemaDecl.cpp
@@ -12554,12 +12554,17 @@
 InitializationSequence InitSeq(*this, Entity, Kind, None);
 ExprResult Init = InitSeq.Perform(*this, Entity, Kind, None);
 
-// If default-init fails, leave var uninitialized but valid, for recovery.
-
 if (Init.get()) {
   Var->setInit(MaybeCreateExprWithCleanups(Init.get()));
   // This is important for template substitution.
   Var->setInitStyle(VarDecl::CallInit);
+} else if (Init.isInvalid()) {
+  // If default-init fails, attach a recovery-expr initializer to track
+  // that initialization was attempted and failed.
+  auto RecoveryExpr =
+  CreateRecoveryExpr(Var->getLocation(), Var->getEndLoc(), {});
+  if (RecoveryExpr.get())
+Var->setInit(RecoveryExpr.get());
 }
 
 CheckCompleteVariableDeclaration(Var);


Index: clang/test/SemaCXX/recovery-default-init.cpp
===
--- /dev/null
+++ clang/test/SemaCXX/recovery-default-init.cpp
@@ -0,0 +1,14 @@
+// RUN: %clang_cc1 %s -fsyntax-only -frecovery-ast -verify -std=c++11
+
+// NOTE: the test can be merged into existing tests once -frecovery-ast is on
+// by default.
+
+struct Foo { // expected-note {{candidate constructor (the implicit copy constructor) not viable}}
+  Foo(int); // expected-note {{candidate constructor not viable}}
+  ~Foo() = delete;
+};
+
+void test() {
+  // we expect the "attempt to use a deleted function" diagnostic is suppressed.
+  Foo foo; // expected-error {{no matching constructor for initialization of}}
+}
Index: clang/lib/Sema/SemaDeclCXX.cpp
===
--- clang/lib/Sema/SemaDeclCXX.cpp
+++ clang/lib/Sema/SemaDeclCXX.cpp
@@ -15003,6 +15003,10 @@
 
 void Sema::FinalizeVarWithDestructor(VarDecl *VD, const RecordType *Record) {
   if (VD->isInvalidDecl()) return;
+  // If initializing the variable failed, don't also diagnose problems with
+  // the desctructor, they're likely related.
+  if (VD->getInit() && VD->getInit()->containsErrors())
+return;
 
   CXXRecordDecl *ClassDecl = cast(Record->getDecl());
   if (ClassDecl->isInvalidDecl()) return;
Index: clang/lib/Sema/SemaDecl.cpp
===
--- clang/lib/Sema/SemaDecl.cpp
+++ clang/lib/Sema/SemaDecl.cpp
@@ -12554,12 +12554,17 @@
 InitializationSequence InitSeq(*this, Entity, Kind, None);
 ExprResult Init = InitSeq.Perform(*this, Entity, Kind, None);
 
-// If default-init fails, leave var uninitialized but valid, for recovery.
-
 if (Init.get()) {
   Var->setInit(MaybeCreateExprWithCleanups(Init.get()));
   // This is important for template substitution.
   Var->setInitStyle(VarDecl::CallInit);
+} else if (Init.isInvalid()) {
+  // If default-init fails, attach a recovery-expr initializer to track
+  // that initialization was attempted and failed.
+  auto RecoveryExpr =
+  

[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 

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

[Sam McCall via Phabricator via cfe-commits]

sammccall added a comment.

In D78100#1981729 , @rsmith wrote:

> In 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.)


Thanks, that's useful. The common cause was our original intuition but I wasn't 
sure if that was an appropriate reason to suppress.




Comment at: clang/lib/Sema/SemaDecl.cpp:11998-12001
 if (Result.isInvalid()) {
   VDecl->setInvalidDecl();
   return;
 }

rsmith wrote:
> Should we attach a `RecoveryExpr` initializer in this case?
This is D78116 (which should probably handle the case above, too)



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

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.



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

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 

[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 , @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



___
cfe-commits mailing list
cfe-commits@lists.llvm.org
https://lists.llvm.org/cgi-bin/mailman/listinfo/cfe-commits

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

[Sam McCall via Phabricator via cfe-commits]

sammccall added subscribers: adamcz, rsmith.
sammccall added a comment.

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?




Comment at: clang/lib/Sema/SemaDeclCXX.cpp:15006
   if (VD->isInvalidDecl()) return;
+  if (VD->getInit() && VD->getInit()->containsErrors())
+return;

This deserves a comment, like "if initializing the variable failed, don't also 
diagnose problems with the desctructor, they're likely related".


Repository:
  rG LLVM Github Monorepo

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

https://reviews.llvm.org/D78100



___
cfe-commits mailing list
cfe-commits@lists.llvm.org
https://lists.llvm.org/cgi-bin/mailman/listinfo/cfe-commits

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

[Haojian Wu via Phabricator via cfe-commits]

hokein updated this revision to Diff 257302.
hokein added a comment.

fix the broken tests.


Repository:
  rG LLVM Github Monorepo

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

https://reviews.llvm.org/D78100

Files:
  clang/lib/Sema/SemaDecl.cpp
  clang/lib/Sema/SemaDeclCXX.cpp
  clang/test/CXX/class.access/p4.cpp
  clang/test/CXX/special/class.ctor/p5-0x.cpp
  clang/test/SemaCXX/cxx0x-deleted-default-ctor.cpp
  clang/test/SemaCXX/virtual-base-used.cpp
  clang/test/SemaObjCXX/arc-0x.mm

Index: clang/test/SemaObjCXX/arc-0x.mm
===
--- clang/test/SemaObjCXX/arc-0x.mm
+++ clang/test/SemaObjCXX/arc-0x.mm
@@ -1,4 +1,4 @@
-// RUN: %clang_cc1 -std=c++11 -fsyntax-only -fobjc-arc -fobjc-runtime-has-weak -fobjc-weak -verify -fblocks -fobjc-exceptions %s
+// RUN: %clang_cc1 -frecovery-ast -std=c++11 -fsyntax-only -fobjc-arc -fobjc-runtime-has-weak -fobjc-weak -verify -fblocks -fobjc-exceptions %s
 
 // "Move" semantics, trivial version.
 void move_it(__strong id &) {
@@ -116,13 +116,13 @@
   // Implicitly-declared special functions of a union are deleted by default if
   // ARC is enabled and the union has an ObjC pointer field.
   union U0 {
-id f0; // expected-note 7 {{'U0' is implicitly deleted because variant field 'f0' is an ObjC pointer}}
+id f0; // expected-note 6 {{'U0' is implicitly deleted because variant field 'f0' is an ObjC pointer}}
   };
 
   union U1 {
-__weak id f0; // expected-note 13 {{'U1' is implicitly deleted because variant field 'f0' is an ObjC pointer}}
+__weak id f0; // expected-note 12 {{'U1' is implicitly deleted because variant field 'f0' is an ObjC pointer}}
 U1() = default; // expected-warning {{explicitly defaulted default constructor is implicitly deleted}} expected-note {{explicitly defaulted function was implicitly deleted here}}
-~U1() = default; // expected-warning {{explicitly defaulted destructor is implicitly deleted}} expected-note 2{{explicitly defaulted function was implicitly deleted here}}
+~U1() = default; // expected-warning {{explicitly defaulted destructor is implicitly deleted}} expected-note {{explicitly defaulted function was implicitly deleted here}}
 U1(const U1 &) = default; // expected-warning {{explicitly defaulted copy constructor is implicitly deleted}} expected-note 2 {{explicitly defaulted function was implicitly deleted here}}
 U1(U1 &&) = default; // expected-warning {{explicitly defaulted move constructor is implicitly deleted}}
 U1 & operator=(const U1 &) = default; // expected-warning {{explicitly defaulted copy assignment operator is implicitly deleted}} expected-note 2 {{explicitly defaulted function was implicitly deleted here}}
@@ -154,15 +154,15 @@
   // functions of the containing class.
   struct S0 {
 union {
-  id f0; // expected-note 7 {{'' is implicitly deleted because variant field 'f0' is an ObjC pointer}}
+  id f0; // expected-note 6 {{'' is implicitly deleted because variant field 'f0' is an ObjC pointer}}
   char f1;
 };
   };
 
   struct S1 {
 union {
-  union { // expected-note 2 {{'S1' is implicitly deleted because variant field '' has a non-trivial}} expected-note 5 {{'S1' is implicitly deleted because field '' has a deleted}}
-id f0; // expected-note 3 {{'' is implicitly deleted because variant field 'f0' is an ObjC pointer}}
+  union { // expected-note 2 {{'S1' is implicitly deleted because variant field '' has a non-trivial}} expected-note 4 {{'S1' is implicitly deleted because field '' has a deleted}}
+id f0; // expected-note 2 {{'' is implicitly deleted because variant field 'f0' is an ObjC pointer}}
 char f1;
   };
   int f2;
@@ -172,7 +172,7 @@
   struct S2 {
 union {
   // FIXME: the note should say 'f0' is causing the special functions to be deleted.
-  struct { // expected-note 7 {{'S2' is implicitly deleted because variant field '' has a non-trivial}}
+  struct { // expected-note 6 {{'S2' is implicitly deleted because variant field '' has a non-trivial}}
 id f0;
 int f1;
   };
@@ -189,18 +189,14 @@
   S1 *x5;
   S2 *x6;
 
-  static union { // expected-error {{call to implicitly-deleted default constructor of}} expected-error {{attempt to use a deleted function}}
-id g0; // expected-note {{default constructor of '' is implicitly deleted because variant field 'g0' is an ObjC pointer}} \
-   // expected-note {{destructor of '' is implicitly deleted because}}
+  static union { // expected-error {{call to implicitly-deleted default constructor of}}
+id g0; // expected-note {{default constructor of '' is implicitly deleted because variant field 'g0' is an ObjC pointer}}
   };
 
-  static union { // expected-error {{call to implicitly-deleted default constructor of}} expected-error {{attempt to use a deleted function}}
-union { // expected-note {{default constructor of '' is implicitly 

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

[Haojian Wu via Phabricator via cfe-commits]

hokein created this revision.
hokein added a reviewer: sammccall.
Herald added a project: clang.

This patch fixes the regression diagnostic, which was introduced in
https://reviews.llvm.org/D77395.


Repository:
  rG LLVM Github Monorepo

https://reviews.llvm.org/D78100

Files:
  clang/lib/Sema/SemaDecl.cpp
  clang/lib/Sema/SemaDeclCXX.cpp
  clang/test/CXX/class.access/p4.cpp
  clang/test/CXX/special/class.ctor/p5-0x.cpp
  clang/test/SemaCXX/cxx0x-deleted-default-ctor.cpp
  clang/test/SemaCXX/virtual-base-used.cpp
  clang/test/SemaObjCXX/arc-0x.mm

Index: clang/test/SemaObjCXX/arc-0x.mm
===
--- clang/test/SemaObjCXX/arc-0x.mm
+++ clang/test/SemaObjCXX/arc-0x.mm
@@ -1,4 +1,4 @@
-// RUN: %clang_cc1 -std=c++11 -fsyntax-only -fobjc-arc -fobjc-runtime-has-weak -fobjc-weak -verify -fblocks -fobjc-exceptions %s
+// RUN: %clang_cc1 -frecovery-ast -std=c++11 -fsyntax-only -fobjc-arc -fobjc-runtime-has-weak -fobjc-weak -verify -fblocks -fobjc-exceptions %s
 
 // "Move" semantics, trivial version.
 void move_it(__strong id &) {
@@ -116,13 +116,13 @@
   // Implicitly-declared special functions of a union are deleted by default if
   // ARC is enabled and the union has an ObjC pointer field.
   union U0 {
-id f0; // expected-note 7 {{'U0' is implicitly deleted because variant field 'f0' is an ObjC pointer}}
+id f0; // expected-note 6 {{'U0' is implicitly deleted because variant field 'f0' is an ObjC pointer}}
   };
 
   union U1 {
-__weak id f0; // expected-note 13 {{'U1' is implicitly deleted because variant field 'f0' is an ObjC pointer}}
+__weak id f0; // expected-note 12 {{'U1' is implicitly deleted because variant field 'f0' is an ObjC pointer}}
 U1() = default; // expected-warning {{explicitly defaulted default constructor is implicitly deleted}} expected-note {{explicitly defaulted function was implicitly deleted here}}
-~U1() = default; // expected-warning {{explicitly defaulted destructor is implicitly deleted}} expected-note 2{{explicitly defaulted function was implicitly deleted here}}
+~U1() = default; // expected-warning {{explicitly defaulted destructor is implicitly deleted}} expected-note {{explicitly defaulted function was implicitly deleted here}}
 U1(const U1 &) = default; // expected-warning {{explicitly defaulted copy constructor is implicitly deleted}} expected-note 2 {{explicitly defaulted function was implicitly deleted here}}
 U1(U1 &&) = default; // expected-warning {{explicitly defaulted move constructor is implicitly deleted}}
 U1 & operator=(const U1 &) = default; // expected-warning {{explicitly defaulted copy assignment operator is implicitly deleted}} expected-note 2 {{explicitly defaulted function was implicitly deleted here}}
@@ -154,15 +154,15 @@
   // functions of the containing class.
   struct S0 {
 union {
-  id f0; // expected-note 7 {{'' is implicitly deleted because variant field 'f0' is an ObjC pointer}}
+  id f0; // expected-note 6 {{'' is implicitly deleted because variant field 'f0' is an ObjC pointer}}
   char f1;
 };
   };
 
   struct S1 {
 union {
-  union { // expected-note 2 {{'S1' is implicitly deleted because variant field '' has a non-trivial}} expected-note 5 {{'S1' is implicitly deleted because field '' has a deleted}}
-id f0; // expected-note 3 {{'' is implicitly deleted because variant field 'f0' is an ObjC pointer}}
+  union { // expected-note 2 {{'S1' is implicitly deleted because variant field '' has a non-trivial}} expected-note 4 {{'S1' is implicitly deleted because field '' has a deleted}}
+id f0; // expected-note 2 {{'' is implicitly deleted because variant field 'f0' is an ObjC pointer}}
 char f1;
   };
   int f2;
@@ -172,7 +172,7 @@
   struct S2 {
 union {
   // FIXME: the note should say 'f0' is causing the special functions to be deleted.
-  struct { // expected-note 7 {{'S2' is implicitly deleted because variant field '' has a non-trivial}}
+  struct { // expected-note 6 {{'S2' is implicitly deleted because variant field '' has a non-trivial}}
 id f0;
 int f1;
   };
@@ -189,18 +189,14 @@
   S1 *x5;
   S2 *x6;
 
-  static union { // expected-error {{call to implicitly-deleted default constructor of}} expected-error {{attempt to use a deleted function}}
-id g0; // expected-note {{default constructor of '' is implicitly deleted because variant field 'g0' is an ObjC pointer}} \
-   // expected-note {{destructor of '' is implicitly deleted because}}
+  static union { // expected-error {{call to implicitly-deleted default constructor of}}
+id g0; // expected-note {{default constructor of '' is implicitly deleted because variant field 'g0' is an ObjC pointer}}
   };
 
-  static union { // expected-error {{call to implicitly-deleted default constructor of}} expected-error {{attempt to use a deleted function}}
-union { // expected-note