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