Page MenuHomePhabricator

Extract ASTDumper to a header file
ClosedPublic

Authored by steveire on May 12 2019, 1:16 PM.

Details

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.

Diff Detail

Repository
rC Clang

Event Timeline

steveire created this revision.May 12 2019, 1:16 PM
Herald added a project: Restricted Project. · View Herald TranscriptMay 12 2019, 1:16 PM
Herald added a subscriber: cfe-commits. · View Herald Transcript

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 added a comment.EditedMay 16 2019, 10:44 AM

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 :).

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.

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?
  1. The follow-up patch https://reviews.llvm.org/D61837#change-x5mxz9Lpijjs adds API to ASTNodeTraverser.
  1. 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/

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.

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

Yup, definitely can see that.

  1. 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.

  1. 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.

  1. 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; }

  1. 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 accepted this revision.May 17 2019, 5:53 AM
  1. 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.

This revision is now accepted and ready to land.May 17 2019, 5:53 AM

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).

This revision was automatically updated to reflect the committed changes.