original Tango::Group inherits from Tango::GroupElement, but as far as I understand "clean API" means also that our public classes do not inherit, so new Tango::Group class is stand alone

Sorry it comes a bit late, but can you share more insights on why we want to get rid of inheritance?

And if this is the way to go shouldn't we do it everywhere inheritance is used?

What I've done here is to preserve old behavior, but if we use set_value for an enum with a short value directly we will have a warning. Maybe it should mention it's a deprecated behavior more explicitly if this is what we decided, but overall this is what we agreed upon isn't it?

We should probably disallow passing a short value to set_value with enum attributes, but that should be done in a separate MR. @dlacoste-esrf to update the implementation here to allow passing short values to set_value for enum attributes as was being done before.

As it is, we preserve the old behavior, and my understanding was that this is what we wanted. Row if we agreed not to allow calling set_value for an enum with a short value, I don't see why we wouldn't do it here… Your 2 sentences are kind of contradictory and now I'm confused!

Thx, but we don't test for shorts! we should keep the first test, with an attribute of type Tango::DevEnum or simply short. For backward compatibility.

Can we use this ticket to trigger a more general discussion on the subject?

At the ESRF we started playing around with debug-prefix-map, here is some interesting info on why it could be a good idea https://reproducible-builds.org/docs/build-path/, maybe we could have a global approach at the tango-controls level to do this kind of stuff.

We started looking into stripped binaries and so on as well. So far we either build a binary without debug symbols, or one with. But there are other approaches, like on debian systems, the binary are stripped of the debug symbols, but upon installing *-dbg packages, it brings extra files just for the debug symbols, not a full binary. This is saving quite some space it seems. I don't know what's the standard way to proceed for conda builds, but I think it would be interesting to be able to offer these kind of possibilities, and at the least discuss this aspect of how we build software in the community!

done, but now aren't we in a pinch for the review?

Damien Lacoste (57f4ddfc) at 20 Mar 07:08

My thoughts exactly: !1592 (b743596a)

While pimpling Attribute and friends I had the same issue, and so the template signature is excluding DevState, otherwise it was not working. So I wouldn't bother about fixing it in the other places, it will be done.

But yes maybe have is_enum_compatible reject DevState is something we want! Our usual procedure would call for another MR as this is getting out of scope, there may be another test to fix and so. I can do it if you want or we squeeze it in this one and be done…

I'm not sure on how and when we use this get_ref_type but there is a little catch with is_enum_compatible, it will return true for DevState. So in here too… And we will get Enumeration where we used to get DevState.

Which is not what we want I guess, but I'm a little surprised it didn't cause any issues…

Damien Lacoste (a0fc9a03) at 19 Mar 15:12

... and 48 more commits

Just a comment since we said we wanted to keep the old behavior, maybe you can duplicate the test rather than replace it. There is value in testing the behavior is kept with an enum attribute used via shorts.

Damien Lacoste (5afba068) at 19 Mar 14:44

Damien Lacoste (2dd8d8f2) at 19 Mar 14:44

... and 21 more commits

ReadersWritersLock was inheriting from omni_mutex, this is gone.

Damien Lacoste (509e5ddf) at 19 Mar 14:42

... and 22 more commits

How would you feel about merging this MR and then trying to solve it properly later?

Not good. If we agree on the proper path for a fix, and I don't have to implement it for libpqxx6 (but I might do it anyway), I'd rather free some time and implement the proper fix right away.

Replying to my own comment, actually I'd like to argue that the test is buggy as we use set_value with a short, which is not an accepted type for enum…

Then this is a change of behavior, so maybe we need to discuss it…

Damien Lacoste (cac0ee78) at 18 Mar 19:03

Damien Lacoste (45c34fa3) at 18 Mar 19:02

Damien Lacoste (6b53760e) at 18 Mar 16:19

... and 30 more commits