[PATCH] D92990: [clangd] Provide suggestions with invalid config keys

[Sam McCall via Phabricator via cfe-commits]

sammccall added a subscriber: hokein.
sammccall added a comment.

Sorry about being slow here.
Just a couple of suggestions to avoid cluttering the config parsing bits too much, and IIUC fixits can be removed.



================
Comment at: clang-tools-extra/clangd/ConfigYAML.cpp:211
+    llvm::SmallVector<llvm::StringRef>
+    unseenKeys(const llvm::SmallSet<std::string, 8> &SeenKeys) const {
+      llvm::SmallVector<llvm::StringRef> Result;
----------------
I think inlining unseenKeys into warnUnknownKeys would be more readable by isolating the mechanics of typo correction into fewer places.


================
Comment at: clang-tools-extra/clangd/ConfigYAML.cpp:219
+
+    static llvm::Optional<llvm::StringRef>
+    bestGuess(llvm::StringRef Search,
----------------
I think we can pull this function out of this class, since it doesn't have much to do with config parsing. (Probably just anonymous above - it's not so reusable since candidates aren't always just strings)


================
Comment at: clang-tools-extra/clangd/ConfigYAML.cpp:291
+  void error(const llvm::Twine &Msg, llvm::SMRange Range,
+             llvm::ArrayRef<llvm::SMFixIt> Fixes = {}) {
     HadError = true;
----------------
please remove the SMFixIt bits, since we don't have any way to use them


================
Comment at: clang-tools-extra/clangd/ConfigYAML.cpp:231
+        unsigned EditDistance = Search.edit_distance(Item, true, MaxEdit);
+        if (EditDistance == MaxEdit) {
+          if (!Result)
----------------
njames93 wrote:
> sammccall wrote:
> > This branch is confusing.
> > It seems to be saying if we have a tie for best, don't return either result. Why is that important/worth any complexity?
> If we have 2 (or more) possible solutions, we can't say for certainty what the user wanted, in which case its best to not suggest anything, in case its incorrect and instead force the user to take an informed look. It doesn't really add much complexity to do this check, And most of this code is copied from somewhere else in llvm as its a pretty common routine. Maybe they could be coalesced into 1 class to reduce duplication,
Well, there isn't *any* case where we can say for certain that we know what the user wanted.

If this were a common library then that shifts the complexity/accuracy tradeoff and justifies a bit of extra work. As it is, the fact that it's copy/pasted doesn't help much with maintaining/reading it. We can keep this, though I don't feel great about it.

(FWIW, these same copy/pasted numbers thresholds are used for clang's typo correction, and we're pretty sure they're not very good - @hokein has tuning them on his backlog)


Repository:
  rG LLVM Github Monorepo

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

https://reviews.llvm.org/D92990