Backport #357: DevEnum support for commands

[Ingvord]

subj

[Ingvord]

Unfortunately, this PR does not compile on Windows... It would be nice if some Windows expert could have a look.

@NexeyaSGara, could you?

[andygotz]

Hi Igor,

does this PR require a change to the IDL file? If so it cannot be merged with the LTS-9 version ...

Andy

[Ingvord]

@andygotz , yes it does. But I do not see the point - why it can not be merged? IDL is hidden from the end user - they will need only to update dependency e.g. as in travis script (to built the project; for binaries this is not even relevant). BTW Appveyor always downloaded latest idl, so windows Tango 9LTS has been compiled with idl 6 all the time.

[t-b]

@Ingvord

IIRC Appveyor complains about

    void set_in_enum_labels(vector<string> &v)
    {
        this->get_ext()->in_enum_labels = v;
    }

    void set_in_enum_labels(vector<string> &&v)
    {
        this->get_ext()->in_enum_labels = move(v);
    }

and it looks like you want to overload on an r-value. While that is supposed to work even with VS I would just write it as

    void set_in_enum_labels(std::vector<std::string> v)
    {
        this->get_ext()->in_enum_labels = std::move(v);
    }

Effective Modern C++ Item 26 and 41 goes into depth about this.

[andygotz]

We have always changed the major version of the library when modifying the IDL to force recompiling of servers and clients because if you don't do this they might crash due to the different structure of the C++ classes generated by IDL.

Have you tested the modified IDL with servers and clients which have not been recompiled?

[Ingvord]

@andygotz , no. I always recompile everything. We can test that before merge.

I mean you should not just replace old .so with the new one, old clients/servers point to old .so, while newly compiled ones to new. So there should not be any intersection between two libraries -> no crash

[bourtemb]

It might work with the changes you introduced because you basically added one structure and one class without changing the older classes but this breaks the way we have done things up to now (increasing major Tango version when introducing a new Device interface in the IDL).
I see also a risk if someone regenerates some code with omniidl with this new IDL.
If one uses a different version of omniidl compared to the one which was used by the already compiled device servers, there is a risk that the generated code might be different and not binary compatible.
This could potentially lead to crashes too if the device servers are not recompiled.
To be on the safe side, I think we should increase TANGO major version when a new Device interface is added in the IDL to enforce users to recompile their device servers if they want to use this version.
We also usually take the opportunity of moving to a new Tango major version to move the methods and fields added in the classes extensions (e.g.: DeviceDataExt) to their corresponding class (e.g.: DeviceData) and empty the ext classes so if we decide to increase the Tango major version, this would be the good time to do this classes extensions cleanup.

[mliszcz]

There was a discussion about appveyor build failures. One of the errors is due to the use of alternative operator syntax in devapi_base.cpp (not instead of !). MSVC simply does not support that syntax [1]. A workaround would be to use if (! is_nil(device_6)) below:

void Connection::resolve_obj_version(const string &corba_name, const Object_var &obj)
{
    //TODO extract template or Macro or class hierarchy, 'cauz this code clearly is a duplication
    device_6 = Device_6::_narrow(obj);

    if (not is_nil(device_6))
    {

See e.g. #555. The same kind of issue was fixed with 49ad354.

[t-b]

@mliszcz Regarding altenative operator syntax:
We could also pass -fno-operator-names to gcc so that you get a warning on linux as well 1.

[mliszcz]

I'd rather be in favor of fixing MSVC quirks instead of disabling valid (standard) code in GCC. According to stackoverflow, /Za or /permissive- should help as well.

Edit:
One argument behind this would be that alternative operators improve code readability (it's easier to miss ! than not) - but it's just my personal opinion.

[t-b]

@mliszcz @Ingvord

/Za is not suggested for C++, see https://docs.microsoft.com/de-de/cpp/build/reference/za-ze-disable-language-extensions?view=vs-2019.

/permissive- is introduced with VS2017 so we would need some compiler version magic to set it. But that could would be a good idea.

[mliszcz]

@t-b thanks for the links. Their explanation why /Za is not recommended is quite funny: "if you explicitly (via flag or .cpp extension) compile C source as C++ source, the behavior may be unexpected".

But I did some searching, it looks like:

• /Za breaks some WinAPI headers (which is expected as WinAPI is for C) - https://github.com/SasLuca/MSVCBug/blob/master/README.md
• /Za breaks some proper C++ code (probably compiles according to earlier standard) - Compilation fails in MSVC with the Microsoft Language Extensions disabled nlohmann/json#1481

If we really want to change any compilation flags in this PR, I'd give /Za a try (on versions where /permissive is not available) and see if anything breaks.

But the simplest workaround is to just change not into ! (let's say for consistency with existing codebase).

[bourtemb]

But the simplest workaround is to just change not into ! (let's say for consistency with existing codebase).

I would go with this simple solution. I think not was introduced because CLion or codacy was suggesting this change as improvement.
Edit: (to improve readability indeed)