[PATCH] D56267: [clangd] Interfaces for writing code actions

[Sam McCall via Phabricator via cfe-commits]

sammccall added a comment.

(Mostly just comments on Tweak, I think the comments on other parts still apply)



================
Comment at: clangd/SourceCode.h:58
 
+/// Return the file location, corresponding to \p P. Note that one should take
+/// care to avoid comparing the result with expansion locations.
----------------
naming nits:
- this should mention "main-file"
- doesn't need to mention "position" as it's just the param type
- 'sourceLoc' seems an odd abbreviation

Maybe `sourceLocationInMainFile`?

(Current name is consistent with `sourceLocToPosition` but that name is bad too...)


================
Comment at: clangd/refactor/Tweak.cpp:38
+namespace {
+const llvm::StringMap<std::function<std::unique_ptr<Tweak>()>> &
+tweaksRegistry() {
----------------
Can we drop this indirection and use the registry directly?


================
Comment at: clangd/refactor/Tweak.cpp:62
+  }
+  // Ensure deterministic order of the results.
+  llvm::sort(Available,
----------------
(nit: I'd make this `operator<` of `Tweak`? e.g. if priority is added, it should be private)


================
Comment at: clangd/refactor/Tweak.h:78
+/// Contains all actions supported by clangd.
+typedef llvm::Registry<Tweak> TweakRegistry;
+
----------------
nit: can we move this to the cpp file and out of the public interface?
(or keep the registry public, but drop prepareTweak[s] and just make callers do it)


================
Comment at: clangd/refactor/Tweak.h:93
+// If prepare() returns true, return the corresponding tweak.
+std::unique_ptr<Tweak> prepareTweak(TweakID ID, const Tweak::Selection &S);
+
----------------
should this return ErrorOr<unique_ptr<Tweak>> so we can treat both missing ID and prepare failed as errors with messages?


Repository:
  rCTE Clang Tools Extra

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

https://reviews.llvm.org/D56267