[PATCH] D57943: [clangd] **Prototype**: clang-tidy-based tweaks

[Haojian Wu via Phabricator via cfe-commits]

hokein added a comment.

> This is an intriguing idea, and is at least useful to prototype new tweaks.
> 
> I'm not sure whether clang-tidy is the ultimately right API to write tweaks:
> 
> it doesn't have the needed constraints to ensure prepare() is fast (e.g. it emits diagnostics and fixes eagerly)
>  the exact set of nodes that it will trigger on may or may not match what we want
>  it doesn't produce context-sensitive titles

Yeah, relying on the checks' implementations is the Achilles' heel of this approach. To reuse the existing code (create fast prototypes), we sacrifice our flexibility unfortunately :(

Another option would be creating a more general `refactoring` library, and refactoring-like clang-tidy and tweaks can be built on it.

> Maybe we should have this behind a flag, and use it to investigate which tweaks should be implemented natively?

It seems to me there are no straight-forward ways to do it since the tweaks' implementations are isolated (maybe via a macro `ENABLE_CLANG_TIDY_TWEAKS` to control whether we should register these tweaks?).



================
Comment at: clangd/refactor/tweaks/ClangTidy.cpp:66
+
+bool ClangTidyTweak::prepare(const Selection &Inputs) {
+  if (!Inputs.ASTSelection.commonAncestor())
----------------
sammccall wrote:
> obviously this is central to the approach, but... prepare needs to be *fast*. It's hard to see how we guarantee that with all this machinery, especially because the match callback contents may change over time.
Most of the checks' callback are just to detect whether the match result is the interesting case, and generate message & FixIt, I'd say it is cheap, and we only run clang-tidy check on the selected AST node, which should be fast.


================
Comment at: clangd/refactor/tweaks/ClangTidy.cpp:83
+  // Run the check on the AST node under the cursor.
+  CTFinder.match(Inputs.ASTSelection.commonAncestor()->ASTNode,
+                 Inputs.AST.getASTContext());
----------------
sammccall wrote:
> looping over all the ancestors might give better results (but leaves less control over performance).
Besides the performance concern, looping over ancestors would extend the clang-tidy check to run on other AST nodes, for the case below, we would also apply the tweak to the ast nodes that are not **selected**.

```
typedef int Foo
ty^pedef int Foo2;
```


================
Comment at: clangd/refactor/tweaks/ClangTidy.cpp:141
+
+  std::string title() const override { return "Covert type to auto"; }
+};
----------------
sammccall wrote:
> this should really specify which type is getting converted - seems like a possible weakness of this approach.
Agree,  we may use the check diagnostic messages (e.g. `use auto when declaring iterators`) as the title, but this depends on the check implementation, probably not an ideal way. 


================
Comment at: unittests/clangd/TweakTests.cpp:231
+  llvm::StringLiteral Input =
+      "void f() { [[unsigned c = static_cast<unsigned>(1);]] }";
+  llvm::StringLiteral Output =
----------------
sammccall wrote:
> this needs to also trigger if you select "unsigned"
this would be nice to have, but it is a weak point of this approach -- relying on the check implementation.


Repository:
  rCTE Clang Tools Extra

CHANGES SINCE LAST ACTION
  https://reviews.llvm.org/D57943/new/

https://reviews.llvm.org/D57943