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

[Sam McCall via Phabricator via cfe-commits]

sammccall 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

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



================
Comment at: clangd/refactor/tweaks/ClangTidy.cpp:45
+/// Limitations:
+///    - checks only match single AST node would work
+///    - checks don't see any preprocessor events
----------------
I can't quite parse this.
"only checks whose matchers match exactly the selected node will work"?

One concern here is that this is basically an implementation detail of a check: checks can choose to e.g. match on a specific node and do a cheap analysis, or match on a higher-level node and do an expensive one. Maybe as long as the specific checks we want work well, it's not an issue. But every check added here needs to be carefully reviewed for performance.


================
Comment at: clangd/refactor/tweaks/ClangTidy.cpp:66
+
+bool ClangTidyTweak::prepare(const Selection &Inputs) {
+  if (!Inputs.ASTSelection.commonAncestor())
----------------
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.


================
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());
----------------
looping over all the ancestors might give better results (but leaves less control over performance).


================
Comment at: clangd/refactor/tweaks/ClangTidy.cpp:141
+
+  std::string title() const override { return "Covert type to auto"; }
+};
----------------
this should really specify which type is getting converted - seems like a possible weakness of this approach.


================
Comment at: unittests/clangd/TweakTests.cpp:231
+  llvm::StringLiteral Input =
+      "void f() { [[unsigned c = static_cast<unsigned>(1);]] }";
+  llvm::StringLiteral Output =
----------------
this needs to also trigger if you select "unsigned"


Repository:
  rCTE Clang Tools Extra

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

https://reviews.llvm.org/D57943