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