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