[refactor] Add the AST source selection component
Needs ReviewPublic

Authored by arphaman on Wed, Jul 5, 6:56 AM.

Details

Reviewers
klimek
vsk
ioeric
Summary

This patch adds the base AST source selection component to the refactoring library. AST selection is represented using a tree of SelectedASTNode values. Each selected node gets its own selection kind, which can actually be None even in the middle of tree (e.g. statement in a macro whose child is in a macro argument). The initial version constructs a "raw" selection tree, without applying filters and canonicalisation operations to the nodes.

Diff Detail

Repository
rL LLVM
arphaman created this revision.Wed, Jul 5, 6:56 AM
arphaman updated this revision to Diff 105269.EditedWed, Jul 5, 7:25 AM

Add a test-case for implicit declarations.

klimek added inline comments.Mon, Jul 17, 4:44 AM
include/clang/Tooling/Refactoring/ASTSelection.h
31

It's unclear what a selection point is. I'd have expected this to mean "overlaps" when first reading it.

69–75

Any reason not to do multiple ranges?

Also, it's not clear from reading the description here why we need the Location.

lib/Tooling/Refactoring/ASTSelection.cpp
101

Why do we always stop traversal?

104

A short comment explaining when this happens would help understand this.

165

Why don't we do this as part of TraverseDecl() in the visitor?

lib/Tooling/Refactoring/SourceLocationUtils.h
1 ↗(On Diff #105269)

I'm somewhat opposed to files having "utils" in the name, as they tend to accumulate too much stuff.
Can we implement those in SourceLocation and SourceManager respectively?

Some nits.

lib/Tooling/Refactoring/ASTSelection.cpp
24

In LLVM, we use the following class layout:

class X {
public:
  void f();

private:
  void g();

  int member;
}
77

I think getSelectedASTNode would be clearer.

91

Use FIXME(<user-id>) instead of TODO in LLVM. Here and elsewhere.

127–130

This seems to be a recurring pattern. Maybe pull into a function?

arphaman added inline comments.Mon, Jul 17, 9:19 AM
include/clang/Tooling/Refactoring/ASTSelection.h
69–75

I guess multiple ranges can be supported in follow-up patches to keep this one simpler.

I'll add it to the comment, but the idea is that the location corresponds to the cursor/right click position in the editor. That means that location doesn't have to be identical to the selection, since you can select something and then right click anywhere in the editor.

lib/Tooling/Refactoring/ASTSelection.cpp
101

False stops traversal, true is the default return value.

klimek added inline comments.Tue, Jul 18, 1:39 AM
lib/Tooling/Refactoring/ASTSelection.cpp
101

Ah, right. Generally, I'd have expected us to stop traversal once we're past the range?

arphaman updated this revision to Diff 107067.Tue, Jul 18, 5:44 AM
arphaman marked 7 inline comments as done.
  • Address review comments.
  • Remove the Location parameter and ContainsSelectionPoint enum value.
  • Stop traversing early when a declaration that ends after the selection range was reached.
include/clang/Tooling/Refactoring/ASTSelection.h
69–75

I've decided to get rid of Location and ContainsSelectionPoint at this level. I will instead handle this editor specific thing at a higher-level and make selection search range only to simplify things at this level.

lib/Tooling/Refactoring/ASTSelection.cpp
101

We already do stop traversal early in findSelectedASTNodes once we match everything that's possible (although if the selection doesn't overlap with anything we will traverse all top level nodes). I see the point though, we might as stop after the range as well. I added the check to do that in the updated patch.

165

I think it's easier to handle the Objective-C @implementation logic here, unless there's some better way that I can't see ATM.

klimek added inline comments.Tue, Jul 18, 6:35 AM
lib/Tooling/Refactoring/ASTSelection.cpp
165

Ok, in that case, can you write a comment at the start of the loop explaining that we basically only do that for the Objective-C @implementation? (I'd also like to understand that better in general, as I have no clue about Obj-C :)

arphaman added inline comments.Tue, Jul 18, 7:03 AM
lib/Tooling/Refactoring/ASTSelection.cpp
165

Hmm, maybe it would be better to move this logic to another layer. Like a wrapper around RecursiveASTVisitor that ensures that iteration occurs in a lexical order. It can then be used by other things that might need this, this code will get simpler and I will be able to test it better.

klimek added inline comments.Tue, Jul 18, 7:08 AM
lib/Tooling/Refactoring/ASTSelection.cpp
165

Great idea! That would be useful in a bunch of use cases.

arphaman updated this revision to Diff 107121.Tue, Jul 18, 9:46 AM

Factor out the lexical ordering code into a new visitor and simplify the implementation of the ast selection visitor

Oops, I just realised that now there's a small bug with early exits. Since we don't actually have true lexical order for declarations in the @implementation we might exit early after visiting a method in the @implementation before a function that's actually written before that method. I will probably constraint early exits to avoid this case.

arphaman updated this revision to Diff 107138.Tue, Jul 18, 10:44 AM

rm early exit bug