[PATCH] D103935: Add Twine support for std::string_view.

[Duncan P. N. Exon Smith via Phabricator via llvm-commits]

dexonsmith added a comment.

In D103935#2881780 <https://reviews.llvm.org/D103935#2881780>, @saugustine wrote:

> In D103935#2881772 <https://reviews.llvm.org/D103935#2881772>, @dexonsmith wrote:
>
>> Seems like valid/important use case to me!
>
> The ODR violation issues would need to be resolved to support this case.

Well, there's also the option of reverting this patch.

> This patch follows on another one that enables Twine support everywhere over std::string. Before that, because strings are trivially convertable to string_views, there was no ambiguity. With that change there is ambiguity in many places, and causes similar problems to the present one for downstream C++ 17 users. Not sure which case should win.

I took a quick look at the patch mentioned in the description, rG92a79dbe91413f685ab19295fc7a6297dbd6c824 <https://reviews.llvm.org/rG92a79dbe91413f685ab19295fc7a6297dbd6c824>, which seems to be just updating MLIR. I imagine there's a way to make those APIs easier to use without breaking LLVM. IMO, a compile-time failure (problem before this patch) is much easier to reason about and fix for a library user than a miscompile (problem after this patch).

What is the ambiguity between? `Twine` also has a constructor directly from `std::string` so it's not obvious to me how that patch introduces ambiguity. If the problem is an LLVM client that's using `std::string_view` instead of `StringRef`, it seems reasonable for the client to explicitly convert to a `StringRef()` to use the APIs. Alternatively, specific APIs can add `std::string_view` wrapper overloads that defer to the `Twine` versions.


Repository:
  rG LLVM Github Monorepo

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

https://reviews.llvm.org/D103935