[PATCH] D124726: Suggest typoed directives in preprocessor conditionals

[Ken Matsui via Phabricator via llvm-commits]

ken-matsui added a comment.

Thank you for your support!

I updated the code, so could you please review this patch again?



================
Comment at: llvm/lib/Support/StringRef.cpp:102
+// Find a similar string in `Candidates`.
+Optional<std::string> StringRef::find_similar_str(const std::vector<StringRef> &Candidates, size_t Dist) const {
+  // We need to check if `rng` has the exact case-insensitive string because the Levenshtein distance match does not
----------------
aaron.ballman wrote:
> ken-matsui wrote:
> > aaron.ballman wrote:
> > > I am less convinced that we want this as a general interface on `StringRef`. I think callers need to decide whether something is similar enough or not. For example, case sensitivity may not matter for this case but it might matter to another case. Others may wish to limit the max edit_distance for each of the candidates for performance reasons, others may not. We already saw there were questions about whether to allow replacements or not. That sort of thing.
> > > 
> > > I think this functionality should be moved to PPDirectives.cpp as a static helper function that's specific to this use. Also, because this returns one of the candidates passed in, and those are a `StringRef`, I think this function should return a `StringRef` -- this removes allocation costs. I'd also drop the `Dist` parameter as the function is no longer intended to be a general purpose one.
> > I am also in favor of it, but where should I put tests for this functionality?
> > Is it possible to write unit tests for static functions implemented in `.cpp` files?
> This is something we wouldn't usually add tests for directly but instead test the behavior of through lit -- the tests you already have exercise this code path and would show if there's a situation where we expect a viable candidate and don't find any. So I'd not worry about test coverage for it in this case.
I see. Thank you!


Repository:
  rG LLVM Github Monorepo

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

https://reviews.llvm.org/D124726