Extract ASTDumper to a header file

[steveire]

Summary

This class has member APIs which are useful to clients. Make it
possible to use those APIs without adding them to dump() member
functions. Doing so does not scale. The optional arguments to dump()
should be designed to be useful in a debugging context.

[steveire]

Format

[aaron.ballman]

I'm not certain where you're planning to go with this change (or is this the only change you're trying to make in this area?), so it's a bit hard to evaluate this patch. Can you explain a bit more about what you're ultimately trying to accomplish?

It might help if I had a better idea of which APIs you thought were ones that would help users (because my only real concern with this change is that the public interface for this class is rather unpleasant).

[steveire]

In D61835#1504663, @aaron.ballman wrote:

I'm not certain where you're planning to go with this change (or is this the only change you're trying to make in this area?), so it's a bit hard to evaluate this patch. Can you explain a bit more about what you're ultimately trying to accomplish?

It might help if I had a better idea of which APIs you thought were ones that would help users (because my only real concern with this change is that the public interface for this class is rather unpleasant).

The reason the ASTDumper class still exists (for the purpose of dumping an AST to stream at least) is that it dumps the {Function,Var,Class}TemplateDecl 'correctly'.

The users of the follow-up patch https://reviews.llvm.org/D61837#change-x5mxz9Lpijjs need that 'correctness', but also need the public API from ASTNodeTraverser on the instance. (That patch also extends the public API for users).

Perhaps some day the stream-dump output can be changed and the ASTDumper class will not be needed anymore. This is not that day :).

[aaron.ballman]

In D61835#1505048, @steveire wrote:

In D61835#1504663, @aaron.ballman wrote:

I'm not certain where you're planning to go with this change (or is this the only change you're trying to make in this area?), so it's a bit hard to evaluate this patch. Can you explain a bit more about what you're ultimately trying to accomplish?

It might help if I had a better idea of which APIs you thought were ones that would help users (because my only real concern with this change is that the public interface for this class is rather unpleasant).

The reason the ASTDumper class still exists (for the purpose of dumping an AST to stream at least) is that it dumps the {Function,Var,Class}TemplateDecl 'correctly'.

Yup, this I recall.

The users of the follow-up patch https://reviews.llvm.org/D61837#change-x5mxz9Lpijjs need that 'correctness', but also need the public API from ASTNodeTraverser on the instance. (That patch also extends the public API for users).

I don't see anything in the follow-up patch that actually uses the ASTDumper class though, so I'm still a bit confused.

Perhaps some day the stream-dump output can be changed and the ASTDumper class will not be needed anymore. This is not that day :).

Yeah, and I'm not suggesting today needs to be that day, either. :-) It's just that this looks like a change with no positive impact because the change isn't being used anywhere (that I can tell), so the only thing I have to go on is "does this produce a decent public API?" and my feeling is "no, this class is an implementation detail and should remain hidden", especially since we think we can remove it someday.

[steveire]

In D61835#1505151, @aaron.ballman wrote:

In D61835#1505048, @steveire wrote:

The users of the follow-up patch https://reviews.llvm.org/D61837#change-x5mxz9Lpijjs need that 'correctness', but also need the public API from ASTNodeTraverser on the instance. (That patch also extends the public API for users).

I don't see anything in the follow-up patch that actually uses the ASTDumper class though, so I'm still a bit confused.

Maybe my wording was confusing. I'll try again:

1. Anyone who wants traversal in the same way that dumping is done must use an instance of the ASTDumper class. Is that much clear? Does my previous response clarify why that is the case?

2. The follow-up patch https://reviews.llvm.org/D61837#change-x5mxz9Lpijjs adds API to ASTNodeTraverser.

3. Anyone who wants traversal in the same way that dumping is done and who needs to call API on the instance which is provided by ASTNodeTraverser (which ASTDumper inherits) needs to use ASTDumper. For example my UI. See my EuroLLVM talk for more: https://steveire.wordpress.com/2019/04/30/the-future-of-ast-matching-refactoring-tools-eurollvm-and-accu/

[aaron.ballman]

In D61835#1505202, @steveire wrote:

In D61835#1505151, @aaron.ballman wrote:

In D61835#1505048, @steveire wrote:

The users of the follow-up patch https://reviews.llvm.org/D61837#change-x5mxz9Lpijjs need that 'correctness', but also need the public API from ASTNodeTraverser on the instance. (That patch also extends the public API for users).

I don't see anything in the follow-up patch that actually uses the ASTDumper class though, so I'm still a bit confused.

Maybe my wording was confusing. I'll try again:

1. Anyone who wants traversal in the same way that dumping is done must use an instance of the ASTDumper class. Is that much clear? Does my previous response clarify why that is the case?

Yup, on the same page there for the most part. Is the "anyone" you refer to someone developing clang (or clang's tools), or third party? I've been envisioning clang development.

2. The follow-up patch https://reviews.llvm.org/D61837#change-x5mxz9Lpijjs adds API to ASTNodeTraverser.

Yup, definitely can see that.

3. Anyone who wants traversal in the same way that dumping is done and who needs to call API on the instance which is provided by ASTNodeTraverser (which ASTDumper inherits) needs to use ASTDumper. For example my UI. See my EuroLLVM talk for more: https://steveire.wordpress.com/2019/04/30/the-future-of-ast-matching-refactoring-tools-eurollvm-and-accu/

Do they? Why is the ASTNodeTraverser insufficient? e.g., can they be given a reference to the base class rather than the derived class? For instance, Decl::dump() creates an ASTDumper instance, but to initiate the traversal, it calls Visit() from the base class.

[steveire]

In D61835#1505228, @aaron.ballman wrote:

In D61835#1505202, @steveire wrote:

3. Anyone who wants traversal in the same way that dumping is done and who needs to call API on the instance which is provided by ASTNodeTraverser (which ASTDumper inherits) needs to use ASTDumper. For example my UI. See my EuroLLVM talk for more: https://steveire.wordpress.com/2019/04/30/the-future-of-ast-matching-refactoring-tools-eurollvm-and-accu/

Do they? Why is the ASTNodeTraverser insufficient?

It doesn't have the same behavior as ASTDumper, because ASTDumper "overrides" some Visit metthods.

[aaron.ballman]

In D61835#1505280, @steveire wrote:

In D61835#1505228, @aaron.ballman wrote:

In D61835#1505202, @steveire wrote:

3. Anyone who wants traversal in the same way that dumping is done and who needs to call API on the instance which is provided by ASTNodeTraverser (which ASTDumper inherits) needs to use ASTDumper. For example my UI. See my EuroLLVM talk for more: https://steveire.wordpress.com/2019/04/30/the-future-of-ast-matching-refactoring-tools-eurollvm-and-accu/

Do they? Why is the ASTNodeTraverser insufficient?

It doesn't have the same behavior as ASTDumper, because ASTDumper "overrides" some Visit metthods.

I'm aware that they're different, but I may not have been sufficiently clear. I'm saying that the only public APIs I think a user should be calling are inherited from ASTNodeTraverser and not ASTDumper, so it is not required to expose ASTDumper directly, only a way to get an ASTNodeTraverser reference/pointer to an ASTDumper instance so that you get the correct virtual dispatch. e.g., ASTNodeTraverser *getSomethingThatActsLikeAnASTDumper() { return new ASTDumper; }

[steveire]

In D61835#1505314, @aaron.ballman wrote:

In D61835#1505280, @steveire wrote:

In D61835#1505228, @aaron.ballman wrote:

In D61835#1505202, @steveire wrote:

3. Anyone who wants traversal in the same way that dumping is done and who needs to call API on the instance which is provided by ASTNodeTraverser (which ASTDumper inherits) needs to use ASTDumper. For example my UI. See my EuroLLVM talk for more: https://steveire.wordpress.com/2019/04/30/the-future-of-ast-matching-refactoring-tools-eurollvm-and-accu/

Do they? Why is the ASTNodeTraverser insufficient?

It doesn't have the same behavior as ASTDumper, because ASTDumper "overrides" some Visit metthods.

I'm aware that they're different, but I may not have been sufficiently clear. I'm saying that the only public APIs I think a user should be calling are inherited from ASTNodeTraverser and not ASTDumper, so it is not required to expose ASTDumper directly, only a way to get an ASTNodeTraverser reference/pointer to an ASTDumper instance so that you get the correct virtual dispatch. e.g., ASTNodeTraverser *getSomethingThatActsLikeAnASTDumper() { return new ASTDumper; }

Perhaps. One way to implement the 'less noise' version of AST output (ie removing pointer addresses etc http://ce.steveire.com/z/HaCLeO ) is to make it an API on the TextNodeDumper. Then ASTDumper would need a forwarding API for it.

Anyway, your argument also applies to JSONNodeDumper, but you put that in a header file. That was sane. We should move ASTDumper to a header similarly. (Perhaps a rename of the class would help though?)

[aaron.ballman]

In D61835#1505388, @steveire wrote:

In D61835#1505314, @aaron.ballman wrote:

In D61835#1505280, @steveire wrote:

In D61835#1505228, @aaron.ballman wrote:

In D61835#1505202, @steveire wrote:

3. Anyone who wants traversal in the same way that dumping is done and who needs to call API on the instance which is provided by ASTNodeTraverser (which ASTDumper inherits) needs to use ASTDumper. For example my UI. See my EuroLLVM talk for more: https://steveire.wordpress.com/2019/04/30/the-future-of-ast-matching-refactoring-tools-eurollvm-and-accu/

Do they? Why is the ASTNodeTraverser insufficient?

It doesn't have the same behavior as ASTDumper, because ASTDumper "overrides" some Visit metthods.

I'm aware that they're different, but I may not have been sufficiently clear. I'm saying that the only public APIs I think a user should be calling are inherited from ASTNodeTraverser and not ASTDumper, so it is not required to expose ASTDumper directly, only a way to get an ASTNodeTraverser reference/pointer to an ASTDumper instance so that you get the correct virtual dispatch. e.g., ASTNodeTraverser *getSomethingThatActsLikeAnASTDumper() { return new ASTDumper; }

Perhaps. One way to implement the 'less noise' version of AST output (ie removing pointer addresses etc http://ce.steveire.com/z/HaCLeO ) is to make it an API on the TextNodeDumper. Then ASTDumper would need a forwarding API for it.

Anyway, your argument also applies to JSONNodeDumper, but you put that in a header file.

Yeah, and I was unhappy about doing so, but the alternative was to put it into ASTDumper.cpp as a local class and that felt even worse.

That was sane. We should move ASTDumper to a header similarly. (Perhaps a rename of the class would help though?)

Yeah, I'd be fine with renaming it to be a bit less general than ASTDumper.

This LGTM. It's not a particularly clean interface, but I can see the utility in exposing it for D62056.

[steveire]

Great, thanks.

I'm going to investigate whether we can move the dump method implementations to their respective class files, and then look into a rename for this to StreamNodeDumper or so (name tbd).

include/clang/AST/ASTDumper.h

+//===--- ASTDumper.h - Dumping implementation for ASTs --------------------===//
+//
+// Part of the LLVM Project, under the Apache License v2.0 with LLVM Exceptions.
+// See https://llvm.org/LICENSE.txt for license information.
+// SPDX-License-Identifier: Apache-2.0 WITH LLVM-exception
+//
+//===----------------------------------------------------------------------===//
+
+#ifndef LLVM_CLANG_AST_ASTDUMPER_H
+#define LLVM_CLANG_AST_ASTDUMPER_H
+
+#include "clang/AST/ASTNodeTraverser.h"
+#include "clang/AST/TextNodeDumper.h"
+
+namespace clang {
+
+class ASTDumper : public ASTNodeTraverser<ASTDumper, TextNodeDumper> {
+
+  TextNodeDumper NodeDumper;
+
+  raw_ostream &OS;
+
+  const bool ShowColors;
+
+public:
+  ASTDumper(raw_ostream &OS, const comments::CommandTraits *Traits,
+            const SourceManager *SM)
+      : ASTDumper(OS, Traits, SM, SM && SM->getDiagnostics().getShowColors()) {}
+
+  ASTDumper(raw_ostream &OS, const comments::CommandTraits *Traits,
+            const SourceManager *SM, bool ShowColors)
+      : ASTDumper(OS, Traits, SM, ShowColors, LangOptions()) {}
+  ASTDumper(raw_ostream &OS, const comments::CommandTraits *Traits,
+            const SourceManager *SM, bool ShowColors,
+            const PrintingPolicy &PrintPolicy)
+      : NodeDumper(OS, ShowColors, SM, PrintPolicy, Traits), OS(OS),
+        ShowColors(ShowColors) {}
+
+  TextNodeDumper &doGetNodeDelegate() { return NodeDumper; }
+
+  void dumpLookups(const DeclContext *DC, bool DumpDecls);
+
+  template <typename SpecializationDecl>
+  void dumpTemplateDeclSpecialization(const SpecializationDecl *D,
+                                      bool DumpExplicitInst, bool DumpRefOnly);
+  template <typename TemplateDecl>
+  void dumpTemplateDecl(const TemplateDecl *D, bool DumpExplicitInst);
+
+  void VisitFunctionTemplateDecl(const FunctionTemplateDecl *D);
+  void VisitClassTemplateDecl(const ClassTemplateDecl *D);
+  void VisitVarTemplateDecl(const VarTemplateDecl *D);
+};
+
+} // namespace clang
+
+#endif

lib/AST/ASTDumper.cpp

 //===--- ASTDumper.cpp - Dumping implementation for ASTs ------------------===//
 //
 // Part of the LLVM Project, under the Apache License v2.0 with LLVM Exceptions.
 // See https://llvm.org/LICENSE.txt for license information.
 // SPDX-License-Identifier: Apache-2.0 WITH LLVM-exception
 //
 //===----------------------------------------------------------------------===//
 //
 // This file implements the AST dump methods, which dump out the
 // AST in a form that exposes type details and other fields.
 //
 //===----------------------------------------------------------------------===//
 
+#include "clang/AST/ASTDumper.h"
 #include "clang/AST/ASTContext.h"
-#include "clang/AST/ASTNodeTraverser.h"
 #include "clang/AST/DeclLookups.h"
 #include "clang/AST/JSONNodeDumper.h"
-#include "clang/AST/TextNodeDumper.h"
 #include "clang/Basic/Builtins.h"
 #include "clang/Basic/Module.h"
 #include "clang/Basic/SourceManager.h"
 #include "llvm/Support/raw_ostream.h"
 using namespace clang;
 using namespace clang::comments;
 
-//===----------------------------------------------------------------------===//
-// ASTDumper Visitor
-//===----------------------------------------------------------------------===//
-
-namespace  {
-
-class ASTDumper : public ASTNodeTraverser<ASTDumper, TextNodeDumper> {
-
-  TextNodeDumper NodeDumper;
-
-  raw_ostream &OS;
-
-  const bool ShowColors;
-
-public:
-  ASTDumper(raw_ostream &OS, const CommandTraits *Traits,
-            const SourceManager *SM)
-      : ASTDumper(OS, Traits, SM, SM && SM->getDiagnostics().getShowColors()) {}
-
-  ASTDumper(raw_ostream &OS, const CommandTraits *Traits,
-            const SourceManager *SM, bool ShowColors)
-      : ASTDumper(OS, Traits, SM, ShowColors, LangOptions()) {}
-  ASTDumper(raw_ostream &OS, const CommandTraits *Traits,
-            const SourceManager *SM, bool ShowColors,
-            const PrintingPolicy &PrintPolicy)
-      : NodeDumper(OS, ShowColors, SM, PrintPolicy, Traits), OS(OS),
-        ShowColors(ShowColors) {}
-
-  TextNodeDumper &doGetNodeDelegate() { return NodeDumper; }
-
-  void dumpLookups(const DeclContext *DC, bool DumpDecls);
-
-  template <typename SpecializationDecl>
-  void dumpTemplateDeclSpecialization(const SpecializationDecl *D,
-                                      bool DumpExplicitInst, bool DumpRefOnly);
-  template <typename TemplateDecl>
-  void dumpTemplateDecl(const TemplateDecl *D, bool DumpExplicitInst);
-
-  void VisitFunctionTemplateDecl(const FunctionTemplateDecl *D);
-  void VisitClassTemplateDecl(const ClassTemplateDecl *D);
-  void VisitVarTemplateDecl(const VarTemplateDecl *D);
-};
-} // namespace
-
 void ASTDumper::dumpLookups(const DeclContext *DC, bool DumpDecls) {
   NodeDumper.AddChild([=] {
     OS << "StoredDeclsMap ";
     NodeDumper.dumpBareDeclRef(cast<Decl>(DC));
 
     const DeclContext *Primary = DC->getPrimaryContext();
     if (Primary != DC) {
       OS << " primary";