Allow string_view as default value for string flags#2059
Allow string_view as default value for string flags#2059DrishtiTripathi2230 wants to merge 1 commit into
Conversation
There was a problem hiding this comment.
So I'm reading what I wrote a few years ago where I said I would consider accepting something like this, but seeing this code is making me think harder about it, and it is making me realize this is a bad idea.
Yes, this code does save a few key strokes. It also unintentionally allows explicit conversions implicitly. We probably could fix that and only allow the implicit conversion for string_view, but what do we gain from it? Saving a few keystrokes is a poor reason to add a convenience wrapper. When you read explicit, verbose C++, you can see exactly what is happening. The justification for a convenience wrapper should be to reduce complexity, not verbosity. We should introduce a utility only when the underlying operation is intrinsically hard to get right.
In short, I think it would be better to use an explicit conversion when you create your flag.
| constexpr T InitDefaultValue(T t) { | ||
| return t; | ||
| template <typename T, typename U = T> | ||
| constexpr T InitDefaultValue(U t) { |
There was a problem hiding this comment.
By changing InitDefaultValue() to use a functional cast, the macro now allows any default value that is constructible to T, even if that constructor is marked explicit. Previously, the default value had to be implicitly convertible to T.
For example, ABSL_FLAG(std::vector<int>, f, 10, "") will now compile and create a vector of size 10. Previously, this would fail because int does not implicitly convert to std::vector.
|
|
||
|
|
||
| // Allow std::string_view to be used as default value for std::string flags | ||
| inline std::string InitDefaultValue(absl::string_view sv) { |
There was a problem hiding this comment.
I think this has no effect because the ABSL_FLAG macro invokes InitDefaultValue with explicit template arguments. The functionality currently works only because the template catch-all allows the explicit conversion from string_view to string.
|
I understand now that this is a bad approach. Thanks for explaining how it can introduce unintended implicit conversions. I'll close this PR. |
Fixes #1540
Allow std::string_view to be used as default value for ABSL_FLAG(std::string, ...).
Changed InitDefaultValue template to accept any convertible type U,
so that string_view is implicitly converted to string when used as
a default value.