tzlaine / parser

A C++ parser combinator library.
Boost Software License 1.0
70 stars 12 forks source link

Strange int-to-string attribute conversions #76

Closed akrzemi1 closed 5 months ago

akrzemi1 commented 5 months ago

I suspect that it is a bug that the following example compiles:

int main()
{
  std::string result;
  auto b = bp::parse("3", bp::int_, bp::ws, result); 
  //                                        ^~~~ storing an int into string
}

Shouldn't some attribute conversion check detect it? This even runs, and returns true. Full example: https://godbolt.org/z/bMv3er6eY

tzlaine commented 5 months ago

Nope. It's just the way std::string works, unfortunately. This is fine:

std::string str;
str = 3; // Implicit int->char conversion.

That's why your example is well-formed. You could also have used bp::double_ with similar results.

akrzemi1 commented 5 months ago

I would encourage you to special-case the int-to-string, double-to-string conversion. Converting anything but char to string is an unintended behavior (maybe except for literal 0). https://www.open-std.org/jtc1/sc22/wg21/docs/papers/2020/p2037r1.html Even assignment from char is fishy: it is like assigning an element to the container of elements. The only reason the committee decided not to fix it is the backwards compatibility. Clang-tidy warns about it.

You will only do your users a favour by disallowing these (even at the cost of special-casing broken std::string. You have the potential to prevent user bugs, without a risk of breaking any valid user code.

tzlaine commented 5 months ago

Ha! I forgot you wrote that paper. I was aware of it. I voted for it IIRC. Yeah, that's fair though. This is super surprising.

akrzemi1 commented 5 months ago

Are you sure this one is fixed? THe example above (https://godbolt.org/z/bMv3er6eY) still compiles, even though I would expect it not to.

tzlaine commented 5 months ago

It's definitely ill-formed on HEAD of master. Maybe the single header file is stale?