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