described enums numeric mode #866
Comments
|
First, what is "'underlying' value"? Second, at first I thought that I forgot to document that conversion happens from described enumerators. But I did document it. So, everything works as documented. Did you forget to describe the enumerators? What are you describing the enum for, if not for enumerators? What is the behaviour you expected? |
|
In declare: "enum class enum_class_use_underlying_type : uint8_t" |
|
That's a type, not a value. What's the behaviour you expected? |
|
Non-descriptive names should be serialized and deserialized as numbers. |
|
Ok, so currently the library behaves as documented. And, I guess you ask for a different behaviour? I'll explain the rationale for our current behaviour. Library-provided conversions are generic and thus try to be the most logical for the types they support. It's logical that for described enums users want to convert to and from strings denoting enumerators. Converting from a random number to some generic enum is problemtaic, because you cannot programmatically determine the allowed range of values for the enum (it's not always the full domain of the underlying type). Thus we decided to not allow such conversion. Converting from an enum to its underlying type is never a problem, and we did not want to throw from So, now that you know the rationale, what's your rationale for describing an enum, but not its enumerators? If you want namespace my_namespace
{
void value_from( ::boost::json::value_from_tag, value& jv, enum_class_use_underlying_type e )
{
using UnderlyingType = std::underlying_type< enum_class_use_underlying_type >::type;
jv = static_cast< UnderlyingType >(e);
}
enum_class_use_underlying_type
value_from( ::boost::json::value_to_tag<enum_class_use_underlying_type>, value const& jv )
{
using UnderlyingType = std::underlying_type< enum_class_use_underlying_type >::type;
auto const n = jv.to_number< UnderlyingType >();
return static_cast<enum_class_use_underlying_type>(n);
}
} |
|
All right. I have to use a solution similar to yours. Yes, I had to enter ranges and/or lists of valid values for "enum". But still there should be a built-in way for the non-string representation of "enum" to ensure compatibility with previous solutions. |
|
What previous solutions? We never provided a generic conversion for described (or any other kind of) enums with a different behaviour. |
|
Suppose we are expecting a string, but there is a number. It is necessary to at least try to return a value, even without checking the validity of the value. Top-level code can additionally check if the value is correct. |
For example, the client and server parts may use different libraries. And for compatibility it is required to be able to work with "enum" as with numbers in JSON. |
|
Ok, so this does not sound like a generic conversion. This is a specific conversion for specific protocol. So, you will have to implement it yourself. Since you already have unearthed implementation for our described enum conversions, you can use it as the basis for your implementation. |
|
I have my own solution, and I have been successfully using it for a long time. But then I think that people would not be confused, it is necessary to remove the direct conversion to a number. |
|
I shared your concern when we implemented this. But currently we don't throw from our generic conversions to |
|
For example, you can do something like this: // described enums
template<class T>
result<T>
value_to_impl(
try_value_to_tag<T>,
value const& jv,
described_enum_conversion_tag)
{
T val = {};
(void)jv;
#ifdef BOOST_DESCRIBE_CXX14
error_code ec;
auto str = jv.if_string();
auto num = jv.if_int64();
auto unum = jv.if_uint64();
if(str)
{
if( !describe::enum_from_string(str->data(), val) )
{
BOOST_JSON_FAIL(ec, error::unknown_name);
return {system::in_place_error, ec};
}
}
else if( num )
{
char const* const name = describe::enum_to_string(*num, nullptr);
if( name )
{
BOOST_JSON_FAIL(ec, error::not_string);
return {system::in_place_error, ec};
}
val = static_cast<T>(*num);
}
else if( unum )
{
char const* const name = describe::enum_to_string(*unum, nullptr);
if( name )
{
BOOST_JSON_FAIL(ec, error::not_string);
return {system::in_place_error, ec};
}
val = static_cast<T>(*unum);
}
else
{
BOOST_JSON_FAIL(ec, error::exhausted_variants);
return {system::in_place_error, ec};
}
#endif
return {system::in_place_value, val};
} |
|
Worse, I think it's not being done. Those who need to check the values for validity will do this separately. |
|
Alternatively, you can add a #define that will serialize and deserialize as a number. In the #define description, you can specify features... |
|
This is undefined behaviour for many enums. And we don't have a way to check if it is or isn't. |
|
Yes, I know. But it comes from compilers and language. You use pointers - although they can also fail. This feature is not easy to take into account - it should not be an obstacle to the operation of the code. |
|
Someday the check will be built into the language, then everything will work as it should. And now it's stupid to stop the development of the code because of this reason. |
|
Those who need more guarantees now can take care of it without your checking. |
|
No, it's completely different from pointers. There's no way to defend against UB in that case, thus we will not implement this behaviour. The point of having generic conversions is that they are uncontroversial. Users can always provide their preferred conversions for their types. |
|
Checks can and do for pointers as well. A value missing from the enum will not crash the program. This value will be treated as other values. Programmers in code may or may not handle other values. It's not bad - it's just a special case. |
|
the only thing to worry about is the size of the type. Might need to add |
|
For example like this: // described enums
template<class T>
result<T>
value_to_impl(
try_value_to_tag<T>,
value const& jv,
described_enum_conversion_tag)
{
T val = {};
(void)jv;
#ifdef BOOST_DESCRIBE_CXX14
error_code ec;
auto str = jv.if_string();
auto num = jv.if_int64();
auto unum = jv.if_uint64();
if(str)
{
if( !describe::enum_from_string(str->data(), val) )
{
BOOST_JSON_FAIL(ec, error::unknown_name);
return {system::in_place_error, ec};
}
}
else if( num )
{
char const* const name = describe::enum_to_string(*num, nullptr);
if( name )
{
BOOST_JSON_FAIL(ec, error::not_string);
return {system::in_place_error, ec};
}
if(*num < std::numeric_limits<std::underlying_type_t<T>>::max() || *num > std::numeric_limits<std::underlying_type_t<T>>::max())
{
BOOST_JSON_FAIL(ec, error::size_mismatch);
return {system::in_place_error, ec};
}
val = static_cast<T>(*num);
}
else if( unum )
{
char const* const name = describe::enum_to_string(*num, nullptr);
if( name )
{
BOOST_JSON_FAIL(ec, error::not_string);
return {system::in_place_error, ec};
}
if(*unum < std::numeric_limits<std::underlying_type_t<T>>::max() || *unum > std::numeric_limits<std::underlying_type_t<T>>::max())
{
BOOST_JSON_FAIL(ec, error::size_mismatch);
return {system::in_place_error, ec};
}
val = static_cast<T>(*unum);
}
BOOST_JSON_FAIL(ec, error::exhausted_variants);
return {system::in_place_error, ec};
#endif
return {system::in_place_value, val};
} |
|
There are actual compiler optimizations that take advantage of the allowed enum range. Your program may crash. It may behave erratically. That's what "undefined behaviour" means. |
|
The above example addresses this issue. |
|
There's no point in explaining to me how to amend the current implementation to get this or that behaviour. I've wrote those functions, I know how to change them. But I'm telling you, we're not adding behaviour that silently enables UB. |
|
No, it does not |
|
|
Explain to me, please, why? |
Please add |
|
Fine. I tried to justify all my thoughts. You decide. The developer is you. |
|
The only other way I can help is to make a branch with a pull request. |
|
There's no way to check if that conversion will be UB or not. If you know how to do it, please change the example I provided (the one with a crashing program) so that it does not crash. Then we can discuss whether your change is possible to integrate into the library. Things you've suggested so far would not have solved the problem, because they do not address the problem in any way. |
|
|
|
I repeat, I can't check the range. Unless I'm missing something. Please, show me in CE how to do that, by making the program to not crash. |
#include <limits>
#include <cstdio>
#include <stdexcept>
enum E {a=1, b};
char const* message(E e) {
switch( e ) {
case a: return "a";
case b: return "b";
//case E(0): return "0"; // this programmer error use sanitazer to detect this
//case E(3): return "3"; // this programmer error use sanitazer to detect this
default: return "something else";
}
}
int main(int argc, char**)
{
try
{
argc += 3;
if(argc < std::numeric_limits<std::underlying_type_t<E> >::min() || argc > std::numeric_limits<std::underlying_type_t<E> >::max())
{
throw std::runtime_error("overflow");
}
E e = static_cast<E>(argc);
std::printf(message(e));
} catch (const std::exception& e) {
std::printf(e.what());
}
return 0;
} |
|
In this library, there is no need to iterate over the values in switch! |
|
You did not catch the the value being outside of enum's range. You just changed the code so that UB would not manifest. So, you don't know how to check the range either. Of course, it's because there's no way to do that. Just to reiterate, I will not add silent undefined behaviour to the library. Users are free to write their own conversions, which can have as much UB as they want. They also may solve the issue by only using enums with fixed underlying types. But as generic conversions have to be generic, there's no acceptable solution on our end. |
|
I think you are wrong. |
|
The code remains transparent within the capabilities of the language and the compiler. |
|
Do you think someone expects that it will be converted to a number, but not back? |
|
It's defined by definition. Because it is documented. I will consider adding a note to make this more explicit. |
|
This is code from the library's user space! char const* message(E e) {
switch( e ) {
case a: return "a";
case b: return "b";
//case E(0): return "0"; // this programmer error use sanitazer to detect this
//case E(3): return "3"; // this programmer error use sanitazer to detect this
default: return "something else";
}
} |
|
"-fstrict-enums" also set by the library user |
|
example of a warning to a library user: |
Maybe a bug in https://godbolt.org/ |
|
Crashes because argc is not set or optimized in emulator in site. Try this. int main(int argc, char**)
{
argc = 0;
E e = static_cast<E>(argc + 3);
std::printf(message(e));
return 0;
}or this int main(int argc, char**)
{
argc = 10;
E e = static_cast<E>(argc + 3);
std::printf(message(e));
return 0;
}it Works. |
|
Please let us know if it worked for you. |
|
I also tried it: #include <limits>
#include <cstdio>
enum E {a=1, b};
char const* message(E e) {
switch( e ) {
case a: return "a";
case b: return "b";
case E(0): return "0";
case E(3): return "3";
default: return "something else";
}
}
int main(int argc, char**)
{
argc = std::numeric_limits<int>::max();
std::printf("%i\n",argc);
std::printf("%i\n",argc+3);
E e = static_cast<E>(argc + 3);
std::printf(message(e));
return 0;
}It works too: |
|
I don't think you'll find a correct case that doesn't work. |
In this specific case not, because the value has been checked to have a corresponding enumerator (via enum_to_string). |
This is OK because 3 is in the range of the enum (which is 0..3 in this case because it has to contain the values 1 and 2). But E(4) would be UB. |
|
The rule is, if the value can't fit in the allowed range of the enum, behavior is undefined. The allowed range is: if the enum has a fixed underlying type, the range is that of the type; otherwise, the range is the range of the hypothetical integral type with the smallest number of bits that can fit all the enumerators. So So in principle, we can check is_scoped_enum (which is implementable as in https://github.com/boostorg/endian/blob/develop/include/boost/endian/detail/is_scoped_enum.hpp), and there's no UB there for any But there's no reliable way to detect is_fixed_enum (E2 above.) I have a trait that kind of does (https://github.com/boostorg/endian/blob/feature/is-fixed-enum/include/boost/endian/detail/is_fixed_enum.hpp) but it requires C++17 and doesn't quite work correctly on all compilers. And there's absolutely no way to obtain the range of E1 (except if it's correctly described, in which case we can compute it from the enumerator descriptors). |
|
Perhaps you need to make implementations for each of the four options. enum E1 { a = 1, b = 5 }; // range is 0..7 // ??? most compilers will choose a chunk of memory that is a multiple of a byte for this type
enum E2: int {}; // range is that of int //(std::is_scoped_enum) does not require discussion, since the solution is proposed
enum class E3: int {}; // range is that of int // does not require discussion, since the solution is proposed
enum class E4 {}; // range is that of int, because enum class always has a fixed underlying type //does not require discussion, since the solution is proposed |
|
It's a good idea to compare values from string descriptions, but it's not clear how to convert to a number then? if( num )
{
char const* const name = describe::enum_to_string(*num, nullptr);
if( name )
{
... |
It's still undefined behavior. Bad things will happen if not today, tomorrow. In fairness, -fsanitize=undefined doesn't diagnose this for both GCC and Clang, and it seems to be unspecified before C++17, not undefined. |
|
to detect the numeric mode, you can use: //default
template<typename T>
constexpr bool need_numeric_mode = false;
//specialization:
template<my_enum_type>
constexpr bool need_numeric_mode = true; |
|
The structure of the code is something like this: // described enums
template<class T>
result<T>
value_to_impl(
try_value_to_tag<T>,
value const& jv,
described_enum_conversion_tag)
{
T val = {};
(void)jv;
#ifdef BOOST_DESCRIBE_CXX14
error_code ec;
if constexpr (need_numeric_mode<T>)
{
auto num = jv.if_int64();
auto unum = jv.if_uint64();
if( num )
{
char const* const name = describe::enum_to_string(*num, nullptr);
if( !name ) // detection unnamed variants!!!
{
BOOST_JSON_FAIL(ec, error::value);
return {system::in_place_error, ec};
}
val = static_cast<T>(*num);
}
else if( unum )
{
char const* const name = describe::enum_to_string(*unum, nullptr);
if( !name )
{
BOOST_JSON_FAIL(ec, error::value);
return {system::in_place_error, ec};
}
val = static_cast<T>(*unum);
}
else
{
BOOST_JSON_FAIL(ec, error::not_number);
return {system::in_place_error, ec};
}
}
else
{
auto str = jv.if_string();
if(!str)
{
BOOST_JSON_FAIL(ec, error::not_string);
return {system::in_place_error, ec};
}
if( !describe::enum_from_string(str->data(), val) )
{
BOOST_JSON_FAIL(ec, error::unknown_name);
return {system::in_place_error, ec};
}
}
#endif
return {system::in_place_value, val};
} |
|
Something similar is needed when converting to a number. |
KostinPavel
changed the title
Version of Boost
1.81
Description
Extraction from "underlying" value does not work. Also not covered are "signed underlying" and "unsigned underlying".
Steps necessary to reproduce the problem
Output:
Because in "boost/json/detail/value_from.hpp" function value_from_helper correct use condition
if( name ):but in "boost/json/detail/value_to.hpp" function value_to_impl not using condition
if( name ):The text was updated successfully, but these errors were encountered: