[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