aaron.ballman added inline comments.
================
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
----------------
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.
Repository:
rG LLVM Github Monorepo
CHANGES SINCE LAST ACTION
https://reviews.llvm.org/D124726/new/
https://reviews.llvm.org/D124726
_______________________________________________
cfe-commits mailing list
cfe-commits@lists.llvm.org
https://lists.llvm.org/cgi-bin/mailman/listinfo/cfe-commits
