Skip to content

Allow string_view as default value for string flags#2059

Closed
DrishtiTripathi2230 wants to merge 1 commit into
abseil:masterfrom
DrishtiTripathi2230:fix-string-view-flag-clean
Closed

Allow string_view as default value for string flags#2059
DrishtiTripathi2230 wants to merge 1 commit into
abseil:masterfrom
DrishtiTripathi2230:fix-string-view-flag-clean

Conversation

@DrishtiTripathi2230
Copy link
Copy Markdown
Contributor

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.

Copy link
Copy Markdown
Member

@derekmauro derekmauro left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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) {
Copy link
Copy Markdown
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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) {
Copy link
Copy Markdown
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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.

@DrishtiTripathi2230
Copy link
Copy Markdown
Contributor Author

I understand now that this is a bad approach. Thanks for explaining how it can introduce unintended implicit conversions. I'll close this PR.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

None yet

Projects

None yet

Development

Successfully merging this pull request may close these issues.

[Bug]: ABSL_FLAG(std::string) does not accept std::string_view as a default value

2 participants