sammccall added a comment.
Cool! Adopting this one as the simplest tweak for "how should tweaks do X"
questions.
This depends on helpers not (yet) present in this patch, so not commenting on
them now.
Need unit tests for tweaks. Something like this should work:
Annotations Test(R"cpp(void foo() {
[[if]] (1) { return; } else { continue; }
})cpp");
auto AST = TestTU(Test.code()).build();
auto Sel = Tweak::Selection::create(Test.Code(), AST, Test.range());
auto T = prepareTweak("SwapIfBranches", Sel);
ASSERT_TRUE(T) << T.takeError();
auto Edits = T.apply(Sel);
ASSERT_TRUE(Edits) << Edits.takeError();
auto After = applyAllReplacements(Test.code(), *Edits);
EXPECT_EQ(After, R"cpp(void foo() {
if (1) { continue; } else { return; }
})cpp");
Probably want to wrap it up into a table driven test once we have a few.
================
Comment at: clangd/refactor/tweaks/SwapIfBranches.cpp:34
+/// After:
+/// if (foo) { continue; } else { return 10; }
+class SwapIfBranches : public Tweak {
----------------
The before/after is useful, we should probably have it for all tweaks if
possible.
It'd also be useful to notate where the cursor can be to trigger the action.
Partly because it forces us to consider this!
e.g. (not sure if this actually matches the logic you want, just an example)
```
Before:
if (foo) { return 10; } else { continue; }
^^^^^^^^^^ ^^^^^^^^ ^
After:
...
```
================
Comment at: clangd/refactor/tweaks/SwapIfBranches.cpp:46
+private:
+ tooling::Replacements Result;
+};
----------------
I think prepare() should just verify:
- cursor is in the right place
- else statement exists and isn't an else if
- both branches are compound statements (for now)
- (maybe) relevant locations (if keyword, else keyword, braces) are not macro
expansions
and then record the relevant source locations (or just the IfStmt*)
We may be able to get away with doing all the work in `prepare()`, but it's not
a good model for future tweaks. (And it is at least somewhat wasteful on a hot
path).
================
Comment at: clangd/refactor/tweaks/SwapIfBranches.cpp:50
+
+class FindIfUnderCursor : public RecursiveASTVisitor<FindIfUnderCursor> {
+public:
----------------
(Mostly this is food for thought for later - we shouldn't try to solve
everything in this patch)
Two efficiency problems here:
- doing one traversal per tweak is wasteful (when we have >1 tweaks, but worth
at least *thinking* about)
- traversing the entire AST rather than just the nodes over the cursor is bad
news (though "over the cursor" may not be trivial to define)
Idea for how to "framework" this problem away:
Add `vector<DynTypedNode> SelectedNodes` to the inputs, the idea being that
this is the stack from the narrowest `Expr` under the cursor to the
`TranslationUnitDecl` at the top.
This can be produced by a RecursiveASTVisitor (that either traverses the whole
AST, or just the bits under the cursor if that's simple enough).
Tweaks would iterate over the nodes from narrowest to broadest, deciding
whether to select this node for processing, continue iterating, or bail out.
Matching in checks are then pretty easy to write, we haven't removed too much
flexibility in flow control, and it's pretty hard to write a slow check.
There are some complications:
- we need access to parents somehow (e.g. consider the case of adding NS
qualifiers, we want to match on names but then traverse up to the containing
declrefexpr to get the nested namespace loc)
- the interesting nodes may be a tree rather than a stack, because nodes
overlap in the source. We could store a postorder traversal... but this makes
e.g. "bail out when you hit a functiondecl" harder - you probably want to bail
out of the current branch only.
- things get complicated when we think about macros - depending on the
semantics we want, it may be hard for the framework to prune parts of the tree
that aren't under the cursor withouth traversing the whole AST.
================
Comment at: clangd/refactor/tweaks/SwapIfBranches.cpp:84
+ // to avoid changing semantics of the code (i.e. dangling else).
+ if (!llvm::dyn_cast_or_null<CompoundStmt>(If->getThen()) ||
+ !llvm::dyn_cast_or_null<CompoundStmt>(If->getElse()))
----------------
isa<CompoundStmt>
Repository:
rCTE Clang Tools Extra
CHANGES SINCE LAST ACTION
https://reviews.llvm.org/D56611/new/
https://reviews.llvm.org/D56611
_______________________________________________
cfe-commits mailing list
cfe-commits@lists.llvm.org
http://lists.llvm.org/cgi-bin/mailman/listinfo/cfe-commits
