Unions fail to compile if they contain multiple strings with different bounds

[banburybill]

I have been blessed with some problematic IDL. Here's a test case:

    union Strings switch(long)
    {
    case 0: string<10> str0;
    case 1: string str1;
    };

Not unreasonably, Cyclone represents both union members as std::string. The generated union code maps to a std::variant containing two items of std::string, which the standard explicitly permits. The getter and setter code references union members by type, e.g. std::get<std::string>(), which fails as this only works if all types in the variant are unique.

I'm guessing this is a variant of #127, fixed in #223. This looks to suggest that buggy std::variant implementations exist which can't handle multiple members of the same type, which is a pity since my first thought was to fix this by switching to access via discriminator rather than type. :-(

[eboasson]

I'm guessing this is a variant of https://github.com/eclipse-cyclonedds/cyclonedds-cxx/issues/127, fixed in https://github.com/eclipse-cyclonedds/cyclonedds-cxx/pull/223.

Yes, I agree. I looks like the code that checks whether two types are the same does so on the IDL type, rather than on the C++ type it is mapped to.

I don't know how easy it will be to fix that test, because it is not a given that string and string<N> both get mapped to std::string: you can override the mapping via options to idlc (from generator.c)

  &(idlc_option_t) {
    IDLC_STRING, { .string = &str_tmpl },
    'f', "string-template", "ns_name::string<...>",
    "Template to use for strings instead of std::string. "
    "(default: std:string)."
  },
  &(idlc_option_t) {
    IDLC_STRING, { .string = &str_inc },
    'f', "string-include", "<header>",
    "Header to include if template for string-template is used.",
  },
  &(idlc_option_t) {
    IDLC_STRING, { .string = &bnd_str_tmpl },
    'f', "bounded-string-template", "ns_name::string<{BOUND} ...>",
    "Template to use for strings instead of std::string. \"{BOUND}\" "
    "tags are replaced by the repective maximum value, other text is copied "
    "verbatim. (default: std::string)"
  },
  &(idlc_option_t) {
    IDLC_STRING, { .string = &bnd_str_inc },
    'f', "bounded-string-include", "<header>",
    "Header to include if template for bounded-string-template is used."
  },

On the upside, that also makes me think there may be a work-around: what if you were to map string<N> to a "proper" bounded string type? The templated type would have to implement the interface of std::string (to the expect the generated code uses it, which is limited), but that way, the problem should disappear.

[banburybill]

Got it. Yes, that would work. And I can see a type implementing a proper bounded string type would be nice.

That being said, how do you feel about PR #516 which extends variant type de-duplication to check the generated cpp11 type as well as the IDL type? As you say, it's not a given that string and string<n> get mapped to the same type, so it seems to me there is no alternative but to check the generated cpp11 type.

[banburybill]

As it stands, the above PR does not deal with typedef'd string types, because the generated C++ code uses the typedef name. So in:

IDL:

typedef string<10> String1;
typedef string String2;

union Strings switch(long)
{
case 0: String1 str0;
case 1: String2 str1;
};

the generated C++ is std::variant<String1, String2>. Both String1 and String2 being std::string, variant access via type fails to compile.

I can see 2 ways forward here.

1. Ensure generated C++ code uses the underlying type, not the typedef.
2. OR switch to using index attribute to the variant. This would only work for C++17 or later. Currently one can build the CXX package with option ENABLE_LEGACY which uses boost::variant and C++11. As boost::variant does not have access by index, this would retain the current issue.

I am minded to try option 2, as this produces C++ code that most reflects the IDL.

[banburybill]

PR #519 implements option 2 above.

[eboasson]

I kinda like your suggestion of using indices, at least in my limited knowledge of C++ vagaries it seems it would eliminate the potentialy thorny question of deciding if two types are equal. Only solving it properly for C++17 also makes sense to me. That would simply mean the C++11 support is somewhat restricted, and it may well be those restrictions never become a problem.

I've pinged the one contributed most of the C++ type mapping and IDLC backend, I trust his judgment much more than my own here 🙂