[PATCH] D65433: [clangd] DefineInline action availability checks

[Alex Lorenz via Phabricator via cfe-commits]

arphaman added inline comments.


================
Comment at: clang-tools-extra/clangd/refactor/tweaks/DefineInline.cpp:192
+  Intent intent() const override { return Intent::Refactor; }
+  std::string title() const override { return "Inline function definition"; }
+
----------------
sammccall wrote:
> kadircet wrote:
> > arphaman wrote:
> > > Sorry, I'm not really familiar with design of tweaks, so this might be a bad question: Is it possible to change the title of the tweak on a per-TU basis. More specifically, some tweaks might want to have different titles for Objective-C actions compared to their C++ siblings.
> > Current design returns a constant string per tweak, but we can change that behaviour easily by making this a function of language options. You can take a look at `ClangdServer::enumerateTweaks` for details.
> This isn't quite true: id() must be constant but title() need not be. See ExpandMacro for an example.
> 
> The only constraint is it shouldn't be terribly expensive (this is why ExpandAuto doesn't include the type in the title, due to an AST bug it requires a new traversal).
> 
> > More specifically, some tweaks might want to have different titles for Objective-C actions compared to their C++ siblings.
> I think this is possible and the right idea. But for completeness I should mention: having multiple Tweak subclasses that share implementation is also possible. In particular in cases where you have two related actions and may want to provide *both* as options (maybe extract function vs method?), they'd need to be separate subclasses. (Or we'd need to change the design a bit)
Oh yes, I think it's probably better to use the `Tweak` subclasses like you said. This will be better for ObjC++, which could have either a C++ or an ObjC tweak available depending on the current AST selection.


Repository:
  rG LLVM Github Monorepo

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

https://reviews.llvm.org/D65433