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

Mon Sep 9 01:02:25 PDT 2019

sammccall 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"; }
+
----------------
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)

Repository:
  rG LLVM Github Monorepo

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

https://reviews.llvm.org/D65433