Page MenuHomePhabricator

[libTooling] Provide overloads of `rewriteDescendants` that operate directly on an AST node.
ClosedPublic

Authored by ymandel on Sep 2 2020, 7:44 AM.

Details

Summary

The new overloads apply directly to a node, like the
clang::ast_matchers::match functions, Rather than generating an
EditGenerator combinator.

Diff Detail

Event Timeline

ymandel created this revision.Sep 2 2020, 7:44 AM
Herald added a project: Restricted Project. · View Herald TranscriptSep 2 2020, 7:44 AM
ymandel requested review of this revision.Sep 2 2020, 7:44 AM
gribozavr2 accepted this revision.Sep 2 2020, 4:29 PM
gribozavr2 added a subscriber: gribozavr2.
gribozavr2 added inline comments.
clang/include/clang/Tooling/Transformer/RewriteRule.h
388

I agree that these functions provide very useful functionality, however I feel uneasy about putting a function that returns an EditGenerator and a function that actually executes the refactoring into the same overload set. The first is a part of a DSL, the second is a regular function. Do you think this is a problem worth solving?

IDK what exactly to suggest though. A namespace for DSL functions like we have in AST matchers?

This revision is now accepted and ready to land.Sep 2 2020, 4:29 PM
ymandel updated this revision to Diff 289622.Sep 2 2020, 7:34 PM

Moved new rewriteDescendants overloads to detail namespace

Herald added a reviewer: rriddle. · View Herald Transcript
Herald added a reviewer: aartbik. · View Herald Transcript
Herald added projects: Restricted Project, Restricted Project, Restricted Project, Restricted Project, Restricted Project. · View Herald Transcript
Herald added a reviewer: Restricted Project. · View Herald Transcript
This revision now requires review to proceed.Sep 2 2020, 7:34 PM
ymandel updated this revision to Diff 289623.Sep 2 2020, 7:36 PM

fix diff base; make small clang-tidy suggested change

ymandel removed projects: Restricted Project, Restricted Project, Restricted Project, Restricted Project, Restricted Project.
ymandel removed subscribers: Restricted Project, lldb-commits, libcxx-commits, llvm-commits.
This revision is now accepted and ready to land.Sep 2 2020, 7:39 PM

Thanks for the review!

clang/include/clang/Tooling/Transformer/RewriteRule.h
388

That's a really good point. These new functions are definitely a part of an (implicit) lower-level library that improves manipulation of the AST directly. We don't have any appropriate library yet for this -- SourceCode is in this spirit but even simpler than RewriteRule, which in fact depends on it.

For now, I'll put these in the detail which is (in some sense) the collection of low level functions for which we need to solve this same problem. WDYT? (I wasn't quite clear about the comparison with ast matchers, since both the matchers DSL and the match functions (on which this is loosely based) are in the same namespace).

gribozavr2 added inline comments.Sep 3 2020, 11:32 AM
clang/include/clang/Tooling/Transformer/RewriteRule.h
388

I wasn't quite clear about the comparison with ast matchers, since both the matchers DSL and the match functions (on which this is loosely based) are in the same namespace

Oh right -- I mistakenly thought that clang::ast_matchers only contains the DSL. I think your choice to put functions into detail looks good.