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
_______________________________________________
cfe-commits mailing list
cfe-commits@lists.llvm.org
https://lists.llvm.org/cgi-bin/mailman/listinfo/cfe-commits
