Skip to content

[AST] Teach TextNodeDumper to print the "implicit" bit for coroutine AST nodes #77311

New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Merged
merged 3 commits into from
Jan 9, 2024

Conversation

hokein
Copy link
Collaborator

@hokein hokein commented Jan 8, 2024

No description provided.

@hokein hokein requested a review from ilya-biryukov January 8, 2024 14:16
@llvmbot llvmbot added clang Clang issues not falling into any other category clang:frontend Language frontend issues, e.g. anything involving "Sema" labels Jan 8, 2024
@llvmbot
Copy link
Member

llvmbot commented Jan 8, 2024

@llvm/pr-subscribers-clang

Author: Haojian Wu (hokein)

Changes

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

3 Files Affected:

  • (modified) clang/include/clang/AST/TextNodeDumper.h (+2)
  • (modified) clang/lib/AST/TextNodeDumper.cpp (+10)
  • (added) clang/test/AST/ast-dump-coroutine.cpp (+46)
diff --git a/clang/include/clang/AST/TextNodeDumper.h b/clang/include/clang/AST/TextNodeDumper.h
index 2f4ed082a0c7ad..dd9ed56fc584a7 100644
--- a/clang/include/clang/AST/TextNodeDumper.h
+++ b/clang/include/clang/AST/TextNodeDumper.h
@@ -252,6 +252,8 @@ class TextNodeDumper
   void VisitGotoStmt(const GotoStmt *Node);
   void VisitCaseStmt(const CaseStmt *Node);
   void VisitReturnStmt(const ReturnStmt *Node);
+  void VisitCoawaitExpr(const CoawaitExpr* Node);
+  void VisitCoreturnStmt(const CoreturnStmt *Node);
   void VisitCompoundStmt(const CompoundStmt *Node);
   void VisitConstantExpr(const ConstantExpr *Node);
   void VisitCallExpr(const CallExpr *Node);
diff --git a/clang/lib/AST/TextNodeDumper.cpp b/clang/lib/AST/TextNodeDumper.cpp
index e8274fcd5cfe9c..d53688ea862e34 100644
--- a/clang/lib/AST/TextNodeDumper.cpp
+++ b/clang/lib/AST/TextNodeDumper.cpp
@@ -1094,6 +1094,16 @@ void clang::TextNodeDumper::VisitReturnStmt(const ReturnStmt *Node) {
   }
 }
 
+void clang::TextNodeDumper::VisitCoawaitExpr(const CoawaitExpr* Node) {
+  if (Node->isImplicit())
+    OS << " implicit";
+}
+
+void clang::TextNodeDumper::VisitCoreturnStmt(const CoreturnStmt *Node) {
+  if (Node->isImplicit())
+    OS << " implicit";
+}
+
 void TextNodeDumper::VisitConstantExpr(const ConstantExpr *Node) {
   if (Node->hasAPValueResult())
     AddChild("value",
diff --git a/clang/test/AST/ast-dump-coroutine.cpp b/clang/test/AST/ast-dump-coroutine.cpp
new file mode 100644
index 00000000000000..f760809f53689a
--- /dev/null
+++ b/clang/test/AST/ast-dump-coroutine.cpp
@@ -0,0 +1,46 @@
+// RUN: %clang_cc1 -triple x86_64-unknown-unknown %s -std=c++20 \
+// RUN:    -fsyntax-only -ast-dump -ast-dump-filter test | FileCheck %s
+
+#include "Inputs/std-coroutine.h"
+
+using namespace std;
+
+struct Task {
+  struct promise_type {
+    std::suspend_always initial_suspend() { return {}; }
+    Task get_return_object() {
+      return std::coroutine_handle<promise_type>::from_promise(*this);
+    }
+    std::suspend_always final_suspend() noexcept { return {}; }
+    std::suspend_always return_void() { return {}; }
+    void unhandled_exception() {}
+
+    auto await_transform(int s) {
+      struct awaiter {
+        promise_type *promise;
+        bool await_ready() { return true; }
+        int await_resume() { return 1; }
+        void await_suspend(std::coroutine_handle<>) {}
+      };
+
+      return awaiter{this};
+    }
+  };
+
+  Task(std::coroutine_handle<promise_type> promise);
+
+  std::coroutine_handle<promise_type> handle;
+};
+
+// Verify the implicit AST nodes for coroutines.
+Task test()  {
+  co_await 1;
+}
+// CHECK:        |-DeclStmt {{.*}}
+// CHECK-NEXT:   | `-VarDecl {{.*}} implicit used __promise
+// CHECK-NEXT:   |   `-CXXConstructExpr {{.*}}
+// CHECK-NEXT:   |-ExprWithCleanups {{.*}} 'void'
+// CHECK-NEXT:   | `-CoawaitExpr {{.*}} 'void' implicit
+//                 ...
+// FIXME: the CoreturnStmt should be marked as implicit
+// CHECK: CoreturnStmt {{.*}} <col:6>

Copy link

github-actions bot commented Jan 8, 2024

✅ With the latest revision this PR passed the C/C++ code formatter.

Copy link
Contributor

@ilya-biryukov ilya-biryukov left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

LGTM, other nodes also do this.
But please take a look at suggestions for improving the test before landing.

// CHECK-NEXT: | `-VarDecl {{.*}} implicit used __promise
// CHECK-NEXT: | `-CXXConstructExpr {{.*}}
// CHECK-NEXT: |-ExprWithCleanups {{.*}} 'void'
// CHECK-NEXT: | `-CoawaitExpr {{.*}} 'void' implicit
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Is this node implicit for initial suspend, for a call to await_transform or both? It would be useful to clarify by matching some child nodes or the dump of CoawaitExpr itself.

Also, could you add matching for both implicit and non-implicit co_await and co_return instances to show that the bit does not incidentally gets set/unset all the time?

Fixing CoreturnStmt in a follow-up does make a lot of sense!

Copy link
Collaborator Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Is this node implicit for initial suspend, for a call to await_transform or both?

This implicit node is just for initial suspend, it is the InitSuspend substmt of the CoroutineBodyStmt.

Added some more tests and matches for the written co_await and co_return statements.

@hokein hokein merged commit 414ea3a into llvm:main Jan 9, 2024
hokein added a commit that referenced this pull request Jan 24, 2024
justinfargnoli pushed a commit to justinfargnoli/llvm-project that referenced this pull request Jan 28, 2024
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
clang:frontend Language frontend issues, e.g. anything involving "Sema" clang Clang issues not falling into any other category
Projects
None yet
Development

Successfully merging this pull request may close these issues.

3 participants