Skip to content
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

[clang] NFCI: improve TemplateArgument and TemplateName dump methods #94905

Merged
merged 1 commit into from
Jun 10, 2024

Conversation

mizvekov
Copy link
Contributor

@mizvekov mizvekov commented Jun 9, 2024

These will work as AST Text node dumpers, as usual, instead of AST printers.

Note that for now, the TemplateName dumper is using the TemplateArgument dumper through an implicit conversion.
This can be fixed in a later pass.

@mizvekov mizvekov self-assigned this Jun 9, 2024
@llvmbot llvmbot added clang Clang issues not falling into any other category clang:frontend Language frontend issues, e.g. anything involving "Sema" labels Jun 9, 2024
@llvmbot
Copy link
Collaborator

llvmbot commented Jun 9, 2024

@llvm/pr-subscribers-clang

Author: Matheus Izvekov (mizvekov)

Changes

These will work as AST Text node dumpers, as usual, instead of AST printers.

Note that for now, the TemplateName dumper is using the TemplateArgument dumper through an implicit conversion.
This can be fixed in a later pass.


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

5 Files Affected:

  • (modified) clang/include/clang/AST/TemplateBase.h (+1-1)
  • (modified) clang/include/clang/AST/TemplateName.h (+1-1)
  • (modified) clang/lib/AST/ASTDumper.cpp (+31)
  • (modified) clang/lib/AST/TemplateBase.cpp (-9)
  • (modified) clang/lib/AST/TemplateName.cpp (-11)
diff --git a/clang/include/clang/AST/TemplateBase.h b/clang/include/clang/AST/TemplateBase.h
index fea2c8ccfee67..0eaa4b0eedb35 100644
--- a/clang/include/clang/AST/TemplateBase.h
+++ b/clang/include/clang/AST/TemplateBase.h
@@ -459,7 +459,7 @@ class TemplateArgument {
              bool IncludeType) const;
 
   /// Debugging aid that dumps the template argument.
-  void dump(raw_ostream &Out) const;
+  void dump(raw_ostream &Out, const ASTContext &Context) const;
 
   /// Debugging aid that dumps the template argument to standard error.
   void dump() const;
diff --git a/clang/include/clang/AST/TemplateName.h b/clang/include/clang/AST/TemplateName.h
index 489fccb2ef74d..988a55acd2252 100644
--- a/clang/include/clang/AST/TemplateName.h
+++ b/clang/include/clang/AST/TemplateName.h
@@ -340,7 +340,7 @@ class TemplateName {
              Qualified Qual = Qualified::AsWritten) const;
 
   /// Debugging aid that dumps the template name.
-  void dump(raw_ostream &OS) const;
+  void dump(raw_ostream &OS, const ASTContext &Context) const;
 
   /// Debugging aid that dumps the template name to standard
   /// error.
diff --git a/clang/lib/AST/ASTDumper.cpp b/clang/lib/AST/ASTDumper.cpp
index c8973fdeda352..869527241f2c8 100644
--- a/clang/lib/AST/ASTDumper.cpp
+++ b/clang/lib/AST/ASTDumper.cpp
@@ -360,3 +360,34 @@ LLVM_DUMP_METHOD void ConceptReference::dump(raw_ostream &OS) const {
   ASTDumper P(OS, Ctx, Ctx.getDiagnostics().getShowColors());
   P.Visit(this);
 }
+
+//===----------------------------------------------------------------------===//
+// TemplateName method implementations
+//===----------------------------------------------------------------------===//
+
+// FIXME: These are using the TemplateArgument dumper.
+LLVM_DUMP_METHOD void TemplateName::dump() const {
+  ASTDumper Dumper(llvm::errs(), /*ShowColors=*/false);
+  Dumper.Visit(*this);
+}
+
+LLVM_DUMP_METHOD void TemplateName::dump(llvm::raw_ostream &OS,
+                                         const ASTContext &Context) const {
+  ASTDumper Dumper(OS, Context, Context.getDiagnostics().getShowColors());
+  Dumper.Visit(*this);
+}
+
+//===----------------------------------------------------------------------===//
+// TemplateArgument method implementations
+//===----------------------------------------------------------------------===//
+
+LLVM_DUMP_METHOD void TemplateArgument::dump() const {
+  ASTDumper Dumper(llvm::errs(), /*ShowColors=*/false);
+  Dumper.Visit(*this);
+}
+
+LLVM_DUMP_METHOD void TemplateArgument::dump(llvm::raw_ostream &OS,
+                                             const ASTContext &Context) const {
+  ASTDumper Dumper(OS, Context, Context.getDiagnostics().getShowColors());
+  Dumper.Visit(*this);
+}
diff --git a/clang/lib/AST/TemplateBase.cpp b/clang/lib/AST/TemplateBase.cpp
index 46f7b79b272ef..2e6839e948d9d 100644
--- a/clang/lib/AST/TemplateBase.cpp
+++ b/clang/lib/AST/TemplateBase.cpp
@@ -577,15 +577,6 @@ void TemplateArgument::print(const PrintingPolicy &Policy, raw_ostream &Out,
   }
 }
 
-void TemplateArgument::dump(raw_ostream &Out) const {
-  LangOptions LO; // FIXME! see also TemplateName::dump().
-  LO.CPlusPlus = true;
-  LO.Bool = true;
-  print(PrintingPolicy(LO), Out, /*IncludeType*/ true);
-}
-
-LLVM_DUMP_METHOD void TemplateArgument::dump() const { dump(llvm::errs()); }
-
 //===----------------------------------------------------------------------===//
 // TemplateArgumentLoc Implementation
 //===----------------------------------------------------------------------===//
diff --git a/clang/lib/AST/TemplateName.cpp b/clang/lib/AST/TemplateName.cpp
index 4fc25cb34803e..11544dbb56e31 100644
--- a/clang/lib/AST/TemplateName.cpp
+++ b/clang/lib/AST/TemplateName.cpp
@@ -360,14 +360,3 @@ const StreamingDiagnostic &clang::operator<<(const StreamingDiagnostic &DB,
   OS.flush();
   return DB << NameStr;
 }
-
-void TemplateName::dump(raw_ostream &OS) const {
-  LangOptions LO;  // FIXME!
-  LO.CPlusPlus = true;
-  LO.Bool = true;
-  print(OS, PrintingPolicy(LO));
-}
-
-LLVM_DUMP_METHOD void TemplateName::dump() const {
-  dump(llvm::errs());
-}

@mizvekov mizvekov changed the title [clang] improve TemplateArgument and TemplateName dump methods [clang] NFCI: improve TemplateArgument and TemplateName dump methods Jun 9, 2024
// TemplateName method implementations
//===----------------------------------------------------------------------===//

// FIXME: These are using the TemplateArgument dumper.
Copy link
Contributor

Choose a reason for hiding this comment

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

I don't think this is a bug, right? Arguably less optimal than it could be.
I think if there is room for improvement the fixme comment should be more detailed

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Well you are going to hit dump on a TemplateName, and it's going to dump a TemplateArgument of template kind, which is misleading and incorrect, but still helpful in the narrow context this is a debugging aid not actually used in production.

Copy link
Contributor

@cor3ntin cor3ntin left a comment

Choose a reason for hiding this comment

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

I'd like to see a more detailed comment, otherwise LGTM.
It's clearly an improvement

These will work as AST Text node dumpers, as usual, instead of AST
printers.

Note that for now, the TemplateName dumper is using the TemplateArgument
dumper through an implicit conversion.
This can be fixed in a later pass.
@mizvekov mizvekov force-pushed the users/mizvekov/clang-ast-dump-template branch from fb159ba to f918649 Compare June 10, 2024 13:29
@mizvekov mizvekov merged commit 3f6fe4e into main Jun 10, 2024
4 of 6 checks passed
@mizvekov mizvekov deleted the users/mizvekov/clang-ast-dump-template branch June 10, 2024 13:29
@HerrCai0907 HerrCai0907 mentioned this pull request Jun 13, 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