Issue #63106: [сlang] Representation of ellipsis in AST #80976

shahidiqbal13 · 2024-02-07T11:26:20Z

Raising for preliminary review (forget about formating)
Fixed the '...' catch all ellipsis in catch stmt
Will fix the failed testcases once we get agree on this
I don't think we need to support for the variadic function support ellipsis token this usecase [void bar(int, ...); ]
See the issue here: [сlang] Representation of ellipsis in AST #63106

Output of the fixed 1st issue (catch(...))

`-FunctionDecl 0x55bdaf78d9a0 <test1.cpp:1:1, line:4:1> line:1:6 foo 'void ()'
  `-CompoundStmt 0x55bdaf78dbb8 <col:12, line:4:1>
    `-CXXTryStmt 0x55bdaf78db98 <line:2:3, line:3:14>
      |-CompoundStmt 0x55bdaf78da90 <line:2:7, col:8>
      `-CXXCatchStmt 0x55bdaf78db78 <line:3:3, col:14>
        |-VarDecl 0x55bdaf78db00 <<invalid sloc>, col:12> col:12 catch_all  '<unknown type>'
        `-CompoundStmt 0x55bdaf78db68 <col:13, col:14>

llvmbot · 2024-02-07T11:26:47Z

@llvm/pr-subscribers-clang

Author: Shahid Iqbal (shahidiqbal13)

Changes

Raising for preliminary review (forget about formating)
Fixed the '...' catch all ellipsis in catch stmt
Will fix the failed testcases once we get agree on this
I don't think we need to support for the variadic function support ellipsis token this usecase [void bar(int, ...); ]
See the issue here: [сlang] Representation of ellipsis in AST #63106

Output of the fixed 1st issue (catch(...))

`-FunctionDecl 0x55bdaf78d9a0 &lt;test1.cpp:1:1, line:4:1&gt; line:1:6 foo 'void ()'
  `-CompoundStmt 0x55bdaf78dbb8 &lt;col:12, line:4:1&gt;
    `-CXXTryStmt 0x55bdaf78db98 &lt;line:2:3, line:3:14&gt;
      |-CompoundStmt 0x55bdaf78da90 &lt;line:2:7, col:8&gt;
      `-CXXCatchStmt 0x55bdaf78db78 &lt;line:3:3, col:14&gt;
        |-VarDecl 0x55bdaf78db00 &lt;&lt;invalid sloc&gt;, col:12&gt; col:12 catch_all  '&lt;unknown type&gt;'
        `-CompoundStmt 0x55bdaf78db68 &lt;col:13, col:14&gt;

Full diff: https://github.com/llvm/llvm-project/pull/80976.diff

7 Files Affected:

(modified) clang/NOTES.txt (+2)
(modified) clang/include/clang/AST/DeclBase.h (+9-2)
(modified) clang/include/clang/Sema/Sema.h (+1-1)
(modified) clang/lib/AST/TextNodeDumper.cpp (+3)
(modified) clang/lib/Parse/ParseStmt.cpp (+11-3)
(modified) clang/lib/Sema/SemaDeclCXX.cpp (+5-2)
(modified) clang/test/AST/ast-dump-stmt.cpp (+1-1)

diff --git a/clang/NOTES.txt b/clang/NOTES.txt
index f06ea8c70cd34..c83dda52a1fc2 100644
--- a/clang/NOTES.txt
+++ b/clang/NOTES.txt
@@ -4,6 +4,8 @@
 
 //===---------------------------------------------------------------------===//
 
+//TESTING git infra//
+
 To time GCC preprocessing speed without output, use:
    "time gcc -MM file"
 This is similar to -Eonly.
diff --git a/clang/include/clang/AST/DeclBase.h b/clang/include/clang/AST/DeclBase.h
index 5b1038582bc67..67f5c7dd1b7f9 100644
--- a/clang/include/clang/AST/DeclBase.h
+++ b/clang/include/clang/AST/DeclBase.h
@@ -301,6 +301,9 @@ class alignas(8) Decl {
   LLVM_PREFERRED_TYPE(bool)
   unsigned Implicit : 1;
 
+  LLVM_PREFERRED_TYPE(bool)
+  unsigned HasCatchEllipsis : 1;
+
   /// Whether this declaration was "used", meaning that a definition is
   /// required.
   LLVM_PREFERRED_TYPE(bool)
@@ -394,7 +397,7 @@ class alignas(8) Decl {
   Decl(Kind DK, DeclContext *DC, SourceLocation L)
       : NextInContextAndBits(nullptr, getModuleOwnershipKindForChildOf(DC)),
         DeclCtx(DC), Loc(L), DeclKind(DK), InvalidDecl(false), HasAttrs(false),
-        Implicit(false), Used(false), Referenced(false),
+        Implicit(false), HasCatchEllipsis(false), Used(false), Referenced(false),
         TopLevelDeclInObjCContainer(false), Access(AS_none), FromASTFile(0),
         IdentifierNamespace(getIdentifierNamespaceForKind(DK)),
         CacheValidAndLinkage(llvm::to_underlying(Linkage::Invalid)) {
@@ -402,7 +405,7 @@ class alignas(8) Decl {
   }
 
   Decl(Kind DK, EmptyShell Empty)
-      : DeclKind(DK), InvalidDecl(false), HasAttrs(false), Implicit(false),
+      : DeclKind(DK), InvalidDecl(false), HasAttrs(false), Implicit(false), HasCatchEllipsis(false),
         Used(false), Referenced(false), TopLevelDeclInObjCContainer(false),
         Access(AS_none), FromASTFile(0),
         IdentifierNamespace(getIdentifierNamespaceForKind(DK)),
@@ -597,6 +600,10 @@ class alignas(8) Decl {
   bool isImplicit() const { return Implicit; }
   void setImplicit(bool I = true) { Implicit = I; }
 
+  /// isCatchEllipsisTok - Indicates whether '...' is present in the catch decl
+  bool isCatchEllipsisTok() const { return HasCatchEllipsis; }
+  void setCatchEllipsisTok(bool I = true) { HasCatchEllipsis = I; }
+
   /// Whether *any* (re-)declaration of the entity was used, meaning that
   /// a definition is required.
   ///
diff --git a/clang/include/clang/Sema/Sema.h b/clang/include/clang/Sema/Sema.h
index 501dc01200a1c..ffc6d2926f85f 100644
--- a/clang/include/clang/Sema/Sema.h
+++ b/clang/include/clang/Sema/Sema.h
@@ -5322,7 +5322,7 @@ class Sema final {
                                      SourceLocation IdLoc,
                                      IdentifierInfo *Id);
 
-  Decl *ActOnExceptionDeclarator(Scope *S, Declarator &D);
+  Decl *ActOnExceptionDeclarator(Scope *S, Declarator &D, bool isCatchAll = false);
 
   StmtResult ActOnCXXCatchBlock(SourceLocation CatchLoc,
                                 Decl *ExDecl, Stmt *HandlerBlock);
diff --git a/clang/lib/AST/TextNodeDumper.cpp b/clang/lib/AST/TextNodeDumper.cpp
index e8274fcd5cfe9..1da20cdfa36a6 100644
--- a/clang/lib/AST/TextNodeDumper.cpp
+++ b/clang/lib/AST/TextNodeDumper.cpp
@@ -272,6 +272,9 @@ void TextNodeDumper::Visit(const Decl *D) {
   if (D->isImplicit())
     OS << " implicit";
 
+  if (D->isCatchEllipsisTok())
+    OS << " catch_all ";
+
   if (D->isUsed())
     OS << " used";
   else if (D->isThisDeclarationReferenced())
diff --git a/clang/lib/Parse/ParseStmt.cpp b/clang/lib/Parse/ParseStmt.cpp
index 2531147c23196..56c1e86afd481 100644
--- a/clang/lib/Parse/ParseStmt.cpp
+++ b/clang/lib/Parse/ParseStmt.cpp
@@ -2698,9 +2698,17 @@ StmtResult Parser::ParseCXXCatchBlock(bool FnCatch) {
     Declarator ExDecl(DS, Attributes, DeclaratorContext::CXXCatch);
     ParseDeclarator(ExDecl);
     ExceptionDecl = Actions.ActOnExceptionDeclarator(getCurScope(), ExDecl);
-  } else
-    ConsumeToken();
-
+  }
+  else {
+  ConsumeToken();
+  // explicitly creating a var of type no-type for '...' and marking it as catch_all
+  ParsedAttributes Attributes(AttrFactory);
+  DeclSpec DS(AttrFactory);
+  Declarator ExDecl(DS, Attributes, DeclaratorContext::BlockLiteral);
+  ParseDeclarator(ExDecl);
+  ExceptionDecl = Actions.ActOnExceptionDeclarator(getCurScope(), ExDecl, true);
+  ExceptionDecl->setCatchEllipsisTok();
+  }
   T.consumeClose();
   if (T.getCloseLocation().isInvalid())
     return StmtError();
diff --git a/clang/lib/Sema/SemaDeclCXX.cpp b/clang/lib/Sema/SemaDeclCXX.cpp
index 3688192e6cbe5..1916d3c3d8ebd 100644
--- a/clang/lib/Sema/SemaDeclCXX.cpp
+++ b/clang/lib/Sema/SemaDeclCXX.cpp
@@ -16983,7 +16983,7 @@ VarDecl *Sema::BuildExceptionDeclaration(Scope *S,
 
 /// ActOnExceptionDeclarator - Parsed the exception-declarator in a C++ catch
 /// handler.
-Decl *Sema::ActOnExceptionDeclarator(Scope *S, Declarator &D) {
+Decl *Sema::ActOnExceptionDeclarator(Scope *S, Declarator &D, bool isCatchAll) {
   TypeSourceInfo *TInfo = GetTypeForDeclarator(D, S);
   bool Invalid = D.isInvalidType();
 
@@ -16994,6 +16994,9 @@ Decl *Sema::ActOnExceptionDeclarator(Scope *S, Declarator &D) {
                                              D.getIdentifierLoc());
     Invalid = true;
   }
+    if (isCatchAll)
+    TInfo = Context.getTrivialTypeSourceInfo(Context.UnknownAnyTy,
+                                             D.getIdentifierLoc());
 
   IdentifierInfo *II = D.getIdentifier();
   if (NamedDecl *PrevDecl = LookupSingleName(S, II, D.getIdentifierLoc(),
@@ -17021,7 +17024,7 @@ Decl *Sema::ActOnExceptionDeclarator(Scope *S, Declarator &D) {
 
   VarDecl *ExDecl = BuildExceptionDeclaration(
       S, TInfo, D.getBeginLoc(), D.getIdentifierLoc(), D.getIdentifier());
-  if (Invalid)
+  if (Invalid && !isCatchAll)
     ExDecl->setInvalidDecl();
 
   // Add the exception declaration into this scope.
diff --git a/clang/test/AST/ast-dump-stmt.cpp b/clang/test/AST/ast-dump-stmt.cpp
index 407584e5b82de..a5a7e9d535c1d 100644
--- a/clang/test/AST/ast-dump-stmt.cpp
+++ b/clang/test/AST/ast-dump-stmt.cpp
@@ -41,7 +41,7 @@ void TestCatch2() {
   try {
   }
 // CHECK-NEXT:    CXXCatchStmt
-// CHECK-NEXT:      NULL
+// CHECK-NEXT:      VarDecl {{.*}} '<unknown type>'
 // CHECK-NEXT:      CompoundStmt
   catch (...) {
   }

github-actions · 2024-02-07T11:35:10Z

⚠️ C/C++ code formatter, clang-format found issues in your code. ⚠️

You can test this locally with the following command:

git-clang-format --diff ec42d547eba5c0ad0bddbecc8902d35383968e78 87d105ac46dfbfdc6a21688069d95425da3443c3 -- clang/include/clang/AST/Decl.h clang/include/clang/Sema/Sema.h clang/lib/AST/JSONNodeDumper.cpp clang/lib/AST/TextNodeDumper.cpp clang/lib/Parse/ParseStmt.cpp clang/lib/Sema/SemaDeclCXX.cpp clang/test/AST/ast-dump-stmt-json.cpp clang/test/AST/ast-dump-stmt.cpp

View the diff from clang-format here.

diff --git a/clang/include/clang/AST/Decl.h b/clang/include/clang/AST/Decl.h
index d6e22ce6c46..e6af9d0d74d 100644
--- a/clang/include/clang/AST/Decl.h
+++ b/clang/include/clang/AST/Decl.h
@@ -1487,7 +1487,6 @@ public:
     NonParmVarDeclBits.EllipsisVar = EV;
   }
 
-
   /// Determine whether this local variable can be used with the named
   /// return value optimization (NRVO).
   ///
diff --git a/clang/include/clang/Sema/Sema.h b/clang/include/clang/Sema/Sema.h
index ffc6d2926f8..0f9418e329e 100644
--- a/clang/include/clang/Sema/Sema.h
+++ b/clang/include/clang/Sema/Sema.h
@@ -5322,7 +5322,8 @@ public:
                                      SourceLocation IdLoc,
                                      IdentifierInfo *Id);
 
-  Decl *ActOnExceptionDeclarator(Scope *S, Declarator &D, bool isCatchAll = false);
+  Decl *ActOnExceptionDeclarator(Scope *S, Declarator &D,
+                                 bool isCatchAll = false);
 
   StmtResult ActOnCXXCatchBlock(SourceLocation CatchLoc,
                                 Decl *ExDecl, Stmt *HandlerBlock);
diff --git a/clang/lib/AST/JSONNodeDumper.cpp b/clang/lib/AST/JSONNodeDumper.cpp
index 2a03ad30dd3..9c092d11cee 100644
--- a/clang/lib/AST/JSONNodeDumper.cpp
+++ b/clang/lib/AST/JSONNodeDumper.cpp
@@ -116,8 +116,8 @@ void JSONNodeDumper::Visit(const Decl *D) {
     JOS.attribute("isReferenced", true);
 
   if (const VarDecl *ND = dyn_cast<VarDecl>(D))
-  if (ND->isEllipsisVariable())
-  JOS.attribute("catch_all", true);
+    if (ND->isEllipsisVariable())
+      JOS.attribute("catch_all", true);
 
   if (const auto *ND = dyn_cast<NamedDecl>(D))
     attributeOnlyIfTrue("isHidden", !ND->isUnconditionallyVisible());
diff --git a/clang/lib/AST/TextNodeDumper.cpp b/clang/lib/AST/TextNodeDumper.cpp
index 056ea399d7e..8ffd6119db5 100644
--- a/clang/lib/AST/TextNodeDumper.cpp
+++ b/clang/lib/AST/TextNodeDumper.cpp
@@ -272,8 +272,8 @@ void TextNodeDumper::Visit(const Decl *D) {
   if (D->isImplicit())
     OS << " implicit";
   if (const VarDecl *ND = dyn_cast<VarDecl>(D))
-  if (ND->isEllipsisVariable())
-  OS << " catch_all";
+    if (ND->isEllipsisVariable())
+      OS << " catch_all";
 
   if (D->isUsed())
     OS << " used";
diff --git a/clang/lib/Parse/ParseStmt.cpp b/clang/lib/Parse/ParseStmt.cpp
index 83f2d451486..3d7d50f0435 100644
--- a/clang/lib/Parse/ParseStmt.cpp
+++ b/clang/lib/Parse/ParseStmt.cpp
@@ -2698,15 +2698,16 @@ StmtResult Parser::ParseCXXCatchBlock(bool FnCatch) {
     Declarator ExDecl(DS, Attributes, DeclaratorContext::CXXCatch);
     ParseDeclarator(ExDecl);
     ExceptionDecl = Actions.ActOnExceptionDeclarator(getCurScope(), ExDecl);
-  }
-  else {
-  CatchLoc = ConsumeToken();
-  // explicitly creating a var of type no-type for '...' and marking it as catch_all
-  ParsedAttributes Attributes(AttrFactory);
-  DeclSpec DS(AttrFactory);
-  Declarator ExDecl(DS, Attributes, DeclaratorContext::BlockLiteral);
-  ParseDeclarator(ExDecl);
-  ExceptionDecl = Actions.ActOnExceptionDeclarator(getCurScope(), ExDecl, true);
+  } else {
+    CatchLoc = ConsumeToken();
+    // explicitly creating a var of type no-type for '...' and marking it as
+    // catch_all
+    ParsedAttributes Attributes(AttrFactory);
+    DeclSpec DS(AttrFactory);
+    Declarator ExDecl(DS, Attributes, DeclaratorContext::BlockLiteral);
+    ParseDeclarator(ExDecl);
+    ExceptionDecl =
+        Actions.ActOnExceptionDeclarator(getCurScope(), ExDecl, true);
   }
   T.consumeClose();
   if (T.getCloseLocation().isInvalid())
diff --git a/clang/lib/Sema/SemaDeclCXX.cpp b/clang/lib/Sema/SemaDeclCXX.cpp
index 93ee1b2e089..31d4355731b 100644
--- a/clang/lib/Sema/SemaDeclCXX.cpp
+++ b/clang/lib/Sema/SemaDeclCXX.cpp
@@ -16994,7 +16994,7 @@ Decl *Sema::ActOnExceptionDeclarator(Scope *S, Declarator &D, bool isCatchAll) {
                                              D.getIdentifierLoc());
     Invalid = true;
   }
-    if (isCatchAll)
+  if (isCatchAll)
     TInfo = Context.getTrivialTypeSourceInfo(Context.UnknownAnyTy,
                                              D.getIdentifierLoc());

cor3ntin · 2024-02-07T13:50:06Z

Using an extra bit for that in Decl (that is the base class of all declaration), seems a bit heavy handed as bits in that part of the astnode are precious.
Did you consider storing the EllipsisLoc in CXXCatchStmt ?

… into b_63016

shahidiqbal13 · 2024-02-26T18:31:48Z

Hi @cor3ntin to review the updated fix, will format it later

ChuanqiXu9

+1 that we should reduce the impact of the patch as much as possible.

Also every time we change the data member of decls and stmts, we need to update the serialization part.

ChuanqiXu9 · 2024-02-27T01:59:52Z

clang/include/clang/AST/Decl.h

+    LLVM_PREFERRED_TYPE(bool)
+    unsigned EllipsisVar : 1;


Should we move this to ParmVarDeclBitfields?

It might impact on the testcases failure , but I will check otherwise it will remain this

ChuanqiXu9 · 2024-02-27T02:00:01Z

clang/include/clang/AST/Decl.h

@@ -1053,6 +1053,10 @@ class VarDecl : public DeclaratorDecl, public Redeclarable<VarDecl> {
    LLVM_PREFERRED_TYPE(bool)
    unsigned ExceptionVar : 1;

+    /// To Check the ellipsis


The comment is not clear

Will do it , its expected

ChuanqiXu9 · 2024-02-27T02:00:22Z

clang/include/clang/AST/Decl.h

+  /// Determine the Ellipsis (...) or not
+  bool isEllipsisVariable() const {
+    return isa<ParmVarDecl>(this) ? false : NonParmVarDeclBits.EllipsisVar;
+  }
+  void setEllipsisVariable(bool EV) {
+    assert(!isa<ParmVarDecl>(this));
+    NonParmVarDeclBits.EllipsisVar = EV;
+  }


Should we move this to ParmVarDecl?

It might impact on the testcases failure , but I will check otherwise it will remain this

ChuanqiXu9 · 2024-02-27T02:00:48Z

clang/lib/AST/JSONNodeDumper.cpp

+  if (const VarDecl *ND = dyn_cast<VarDecl>(D))
+  if (ND->isEllipsisVariable())
+  JOS.attribute("catch_all", true);


Format this.

ChuanqiXu9 · 2024-02-27T02:00:57Z

clang/lib/AST/TextNodeDumper.cpp

+  if (const VarDecl *ND = dyn_cast<VarDecl>(D))
+  if (ND->isEllipsisVariable())
+  OS << " catch_all";


ChuanqiXu9 · 2024-02-27T02:06:27Z

clang/lib/Sema/SemaDeclCXX.cpp

@@ -16983,7 +16983,7 @@ VarDecl *Sema::BuildExceptionDeclaration(Scope *S,

 /// ActOnExceptionDeclarator - Parsed the exception-declarator in a C++ catch
 /// handler.
-Decl *Sema::ActOnExceptionDeclarator(Scope *S, Declarator &D) {
+Decl *Sema::ActOnExceptionDeclarator(Scope *S, Declarator &D, bool isCatchAll) {


I am hesitate on this. IIUC, previously the catch(...) format is reprensented as the exception decl as nullptr? Then if yes, we should be able to reuse that.

I think it won't be possible since if you see the ActOnExceptionDeclarator , we are not passing the Decl*, later we are updating the Decl*

ChuanqiXu9 · 2024-02-27T02:06:41Z

clang/lib/Parse/ParseStmt.cpp

+  else {
+  CatchLoc = ConsumeToken();
+  // explicitly creating a var of type no-type for '...' and marking it as catch_all
+  ParsedAttributes Attributes(AttrFactory);
+  DeclSpec DS(AttrFactory);
+  Declarator ExDecl(DS, Attributes, DeclaratorContext::BlockLiteral);
+  ParseDeclarator(ExDecl);
+  ExceptionDecl = Actions.ActOnExceptionDeclarator(getCurScope(), ExDecl, true);
+  }


I am hesitate on this. IIUC, previously the catch(...) format is reprensented as the exception decl as nullptr? Then if yes, we should be able to reuse that.

Hi @ChuanqiXu9 , Thanks for the review!
I think it won't be possible since if you see the ActOnExceptionDeclarator , we are not passing the Decl*, later we are updating the Decl*

ChuanqiXu9 · 2024-02-27T02:06:55Z

clang/test/AST/ast-dump-stmt.cpp

@@ -41,7 +41,7 @@ void TestCatch2() {
  try {
  }
 // CHECK-NEXT:    CXXCatchStmt
-// CHECK-NEXT:      NULL
+// CHECK-NEXT:      VarDecl {{.*}} '<unknown type>'


Maybe we can improve to print this.

Can't do much here , its intently being created the var of unknown type

cor3ntin · 2024-02-27T12:08:38Z

Using an extra bit for that in Decl (that is the base class of all declaration), seems a bit heavy handed as bits in that part of the astnode are precious.
Did you consider storing the EllipsisLoc in CXXCatchStmt ?

Please address this question first. Thanks!

shahidiqbal13 · 2024-02-27T15:28:44Z

Hi @cor3ntin ,
I tried but didn't work as per design, Can you pls review this , I m not updating the Decl , updating the VarDecl

AaronBallman · 2024-03-01T15:48:16Z

Hi @cor3ntin , I tried but didn't work as per design, Can you pls review this , I m not updating the Decl , updating the VarDecl

I'd like more information on what was tried and why it didn't work. It would be better for us to associate the data with catch statements specifically because there's a lot fewer of those than there are variable declarations in general.

shahidiqbal13 · 2024-03-04T13:50:30Z

Hi @AaronBallman
Can you pls explain me your previous response "It would be better for us to associate the data with catch statements specifically because there's a lot fewer of those than there are variable declarations in general."

I mean storing the ellipsis's location in catch stmt is not going to work, we need to add certain flags/function in VarDecl. Can you please review which I have raised currently one more time?

AaronBallman · 2024-03-04T15:02:05Z

Hi @AaronBallman Can you pls explain me your previous response "It would be better for us to associate the data with catch statements specifically because there's a lot fewer of those than there are variable declarations in general."

I mean storing the ellipsis's location in catch stmt is not going to work, we need to add certain flags/function in VarDecl. Can you please review which I have raised currently one more time?

CXXCatchStmt currently stores three things: the location of the catch keyword, the Stmt for the handler, and the VarDecl for the exception declaration. We currently represent a catch-all by setting VarDecl to null. What @Endilll was pointing out in the original issue is that walking the AST for a CXXCatchStmt causes us to walk both of those child nodes, including the null VarDecl in the case of a catch-all.

I was suggesting that we should not walk a null node in that case (so modify the various AST walkers to check for a null pointer in that case and not walk down it), and instead we should have CXXCatchStmt retain a SourceLocation for the ellipsis in the case where the VarDecl is null. This way, people who need to know "is this catch statement a catch-all" can still find where to print diagnostics related to the ..., but we don't end up walking over a null node.

You're approaching this from a different angle, where we have a non-null VarDecl that we use to represent the .... We could go this route and there's some appeal to doing so, but I think in that case, we'd want a concrete AST node to represent it. e.g., we'd want to add class EllipsisDecl : public VarDecl {}; that could then be used by CXXCatchStmt and FunctionDecl, etc. However, this is a more involved approach and requires additional thought as to whether it's worth the extra effort.

CC @erichkeane for additional opinions.

erichkeane · 2024-03-04T15:24:33Z

Hi @AaronBallman Can you pls explain me your previous response "It would be better for us to associate the data with catch statements specifically because there's a lot fewer of those than there are variable declarations in general."
I mean storing the ellipsis's location in catch stmt is not going to work, we need to add certain flags/function in VarDecl. Can you please review which I have raised currently one more time?

CXXCatchStmt currently stores three things: the location of the catch keyword, the Stmt for the handler, and the VarDecl for the exception declaration. We currently represent a catch-all by setting VarDecl to null. What @Endilll was pointing out in the original issue is that walking the AST for a CXXCatchStmt causes us to walk both of those child nodes, including the null VarDecl in the case of a catch-all.

I was suggesting that we should not walk a null node in that case (so modify the various AST walkers to check for a null pointer in that case and not walk down it), and instead we should have CXXCatchStmt retain a SourceLocation for the ellipsis in the case where the VarDecl is null. This way, people who need to know "is this catch statement a catch-all" can still find where to print diagnostics related to the ..., but we don't end up walking over a null node.

You're approaching this from a different angle, where we have a non-null VarDecl that we use to represent the .... We could go this route and there's some appeal to doing so, but I think in that case, we'd want a concrete AST node to represent it. e.g., we'd want to add class EllipsisDecl : public VarDecl {}; that could then be used by CXXCatchStmt and FunctionDecl, etc. However, this is a more involved approach and requires additional thought as to whether it's worth the extra effort.

CC @erichkeane for additional opinions.

I VERY much don't think the extra decl is worth the effort. The Ellipsis Location should just be stored on the CXXCatchStmt, and we can leave the the empty VarDecl alone. Avoiding traversal is a possibility, though not necessary to this approach.

shahidiqbal13 and others added 4 commits November 16, 2023 11:26

TESTING infra

b6bfb18

Merge branch 'llvm:main' into main

ddd7bb1

Merge branch 'llvm:main' into main

95ea682

Issue llvm#63106: [сlang] Representation of ellipsis in AST

79c5928

llvmbot added clang Clang issues not falling into any other category clang:frontend Language frontend issues, e.g. anything involving "Sema" labels Feb 7, 2024

shahidiqbal13 self-assigned this Feb 7, 2024

shahidiqbal13 requested review from AaronBallman, cor3ntin and ChuanqiXu9 February 7, 2024 11:27

Update NOTES.txt

133a67c

shahidiqbal13 linked an issue Feb 7, 2024 that may be closed by this pull request

[сlang] Representation of ellipsis in AST #63106

Open

shahidiqbal13 added 2 commits February 26, 2024 13:20

Updated fix with passed test cases

9440854

Merge branch 'b_63016' of https://github.com/shahidiqbal13/llvm-project…

87d105a

… into b_63016

ChuanqiXu9 reviewed Feb 27, 2024

View reviewed changes

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Issue #63106: [сlang] Representation of ellipsis in AST #80976

Issue #63106: [сlang] Representation of ellipsis in AST #80976

shahidiqbal13 commented Feb 7, 2024

llvmbot commented Feb 7, 2024

github-actions bot commented Feb 7, 2024 •

edited

cor3ntin commented Feb 7, 2024

shahidiqbal13 commented Feb 26, 2024

ChuanqiXu9 left a comment

ChuanqiXu9 Feb 27, 2024

shahidiqbal13 Feb 27, 2024

ChuanqiXu9 Feb 27, 2024

shahidiqbal13 Feb 27, 2024

ChuanqiXu9 Feb 27, 2024

shahidiqbal13 Feb 27, 2024

ChuanqiXu9 Feb 27, 2024

shahidiqbal13 Feb 27, 2024

ChuanqiXu9 Feb 27, 2024

shahidiqbal13 Feb 27, 2024

ChuanqiXu9 Feb 27, 2024

shahidiqbal13 Feb 27, 2024

ChuanqiXu9 Feb 27, 2024

shahidiqbal13 Feb 27, 2024

ChuanqiXu9 Feb 27, 2024

shahidiqbal13 Feb 27, 2024

cor3ntin commented Feb 27, 2024

shahidiqbal13 commented Feb 27, 2024

AaronBallman commented Mar 1, 2024

shahidiqbal13 commented Mar 4, 2024

AaronBallman commented Mar 4, 2024

erichkeane commented Mar 4, 2024

Issue #63106: [сlang] Representation of ellipsis in AST #80976

Are you sure you want to change the base?

Issue #63106: [сlang] Representation of ellipsis in AST #80976

Conversation

shahidiqbal13 commented Feb 7, 2024

llvmbot commented Feb 7, 2024

github-actions bot commented Feb 7, 2024 • edited

cor3ntin commented Feb 7, 2024

shahidiqbal13 commented Feb 26, 2024

ChuanqiXu9 left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

cor3ntin commented Feb 27, 2024

shahidiqbal13 commented Feb 27, 2024

AaronBallman commented Mar 1, 2024

shahidiqbal13 commented Mar 4, 2024

AaronBallman commented Mar 4, 2024

erichkeane commented Mar 4, 2024

github-actions bot commented Feb 7, 2024 •

edited