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