This suggestion really exposes a hole in this refactor. 🤔

The issue is that we don't really want users to have to know or care about whether the string held in the variant is a std::string or a std::string_view. Ideally users would just use value_if<std::string_view>() since it returns a std::optional<std::string> which works for either type.

But get_if<> returns a pointer, not an optional. So generic calls to get_if<std::string_view>() aren't going to work when there's not a std::string_view to point to. These suggestions on line 171 and 174 work since we have the index in hand ... but usually we don't want client code to have to think about these things.

Maybe it makes more sense to close this PR and keep StringHolder since it guarantees we'll always have a pointer to a std::string_view s.t. get_if<std::string_view>() is safe.

Feedback welcomed, but I'm moging this to 'draft' form for now.

Hold on hold on, the intention of this suggestion is just to re-use code that has been implemented by other methods, there is no change in logic or return types and whatnot.

I like what you did in this PR, I don't think we should close it.

IMO we should even go a step further and remove make_visit_adapter() so that we won't need to worry about all that cv/ref mess. Users of the visit() method will have the freedom to implement the std::string handler as an alias of the std::string_view handler, or have separate handling for managed/unmanaged strings. But this can come in another PR.

fyi, this is the method that will be called with my suggestion, should be easy to see that it is equivalent to your code.

I didn't explain myself well. I agree that your two suggestions will work since they specifically look at the index value.

My concern is more about generic code like this, which is risky now in this branch:

std::string_view const* sv = map->find_if<std::string_view>(TR_KEY_download_dir);

Before this PR, this call always worked whether or not the string was managed or unmanaged. With this PR, the user has to know whether to call find_if<std::string>() or find_if<std::string_view>(). IMO this is too error-prone to ship.

Just thinking out loud -- this may also be a case of premature optimization. How many strings do we really go through? And how many unmanaged strings? Maybe we can just use std::string everywhere. I'll do some benchmarks.

Oof no we get quite a benefit from unmanaged strings. If anything, we should consider doing tr_variant changes in-place in api_compat::convert() to avoid making more managed copies. 💀

Happily, 2c49783 should fix the risky API I mentioned in the previous comment. Ready for re-review.